Fix context cache bug & enable context cache for dashabord commits' authors (#26991)

Unfortunately, when a system setting hasn't been stored in the database,
it cannot be cached.
Meanwhile, this PR also uses context cache for push email avatar display
which should avoid to read user table via email address again and again.

According to my local test, this should reduce dashboard elapsed time
from 150ms -> 80ms .
This commit is contained in:
Lunny Xiao 2023-09-11 18:14:01 +08:00 committed by GitHub
parent 598465eb43
commit ebff0513db
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
8 changed files with 72 additions and 87 deletions

View file

@ -153,7 +153,12 @@ func generateEmailAvatarLink(ctx context.Context, email string, size int, final
return DefaultAvatarLink() return DefaultAvatarLink()
} }
enableFederatedAvatar := system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureEnableFederatedAvatar) disableGravatar := system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar,
setting.GetDefaultDisableGravatar(),
)
enableFederatedAvatar := system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureEnableFederatedAvatar,
setting.GetDefaultEnableFederatedAvatar(disableGravatar))
var err error var err error
if enableFederatedAvatar && system_model.LibravatarService != nil { if enableFederatedAvatar && system_model.LibravatarService != nil {
@ -174,7 +179,6 @@ func generateEmailAvatarLink(ctx context.Context, email string, size int, final
return urlStr return urlStr
} }
disableGravatar := system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar)
if !disableGravatar { if !disableGravatar {
// copy GravatarSourceURL, because we will modify its Path. // copy GravatarSourceURL, because we will modify its Path.
avatarURLCopy := *system_model.GravatarSourceURL avatarURLCopy := *system_model.GravatarSourceURL

View file

@ -1,6 +1,6 @@
- -
id: 1 id: 1
setting_key: 'disable_gravatar' setting_key: 'picture.disable_gravatar'
setting_value: 'false' setting_value: 'false'
version: 1 version: 1
created: 1653533198 created: 1653533198
@ -8,7 +8,7 @@
- -
id: 2 id: 2
setting_key: 'enable_federated_avatar' setting_key: 'picture.enable_federated_avatar'
setting_value: 'false' setting_value: 'false'
version: 1 version: 1
created: 1653533198 created: 1653533198

View file

@ -94,11 +94,14 @@ func GetSetting(ctx context.Context, key string) (*Setting, error) {
const contextCacheKey = "system_setting" const contextCacheKey = "system_setting"
// GetSettingWithCache returns the setting value via the key // GetSettingWithCache returns the setting value via the key
func GetSettingWithCache(ctx context.Context, key string) (string, error) { func GetSettingWithCache(ctx context.Context, key, defaultVal string) (string, error) {
return cache.GetWithContextCache(ctx, contextCacheKey, key, func() (string, error) { return cache.GetWithContextCache(ctx, contextCacheKey, key, func() (string, error) {
return cache.GetString(genSettingCacheKey(key), func() (string, error) { return cache.GetString(genSettingCacheKey(key), func() (string, error) {
res, err := GetSetting(ctx, key) res, err := GetSetting(ctx, key)
if err != nil { if err != nil {
if IsErrSettingIsNotExist(err) {
return defaultVal, nil
}
return "", err return "", err
} }
return res.SettingValue, nil return res.SettingValue, nil
@ -108,17 +111,21 @@ func GetSettingWithCache(ctx context.Context, key string) (string, error) {
// GetSettingBool return bool value of setting, // GetSettingBool return bool value of setting,
// none existing keys and errors are ignored and result in false // none existing keys and errors are ignored and result in false
func GetSettingBool(ctx context.Context, key string) bool { func GetSettingBool(ctx context.Context, key string, defaultVal bool) (bool, error) {
s, _ := GetSetting(ctx, key) s, err := GetSetting(ctx, key)
if s == nil { switch {
return false case err == nil:
}
v, _ := strconv.ParseBool(s.SettingValue) v, _ := strconv.ParseBool(s.SettingValue)
return v return v, nil
case IsErrSettingIsNotExist(err):
return defaultVal, nil
default:
return false, err
}
} }
func GetSettingWithCacheBool(ctx context.Context, key string) bool { func GetSettingWithCacheBool(ctx context.Context, key string, defaultVal bool) bool {
s, _ := GetSettingWithCache(ctx, key) s, _ := GetSettingWithCache(ctx, key, strconv.FormatBool(defaultVal))
v, _ := strconv.ParseBool(s) v, _ := strconv.ParseBool(s)
return v return v
} }
@ -259,52 +266,41 @@ var (
) )
func Init(ctx context.Context) error { func Init(ctx context.Context) error {
var disableGravatar bool disableGravatar, err := GetSettingBool(ctx, KeyPictureDisableGravatar, setting_module.GetDefaultDisableGravatar())
disableGravatarSetting, err := GetSetting(ctx, KeyPictureDisableGravatar) if err != nil {
if IsErrSettingIsNotExist(err) {
disableGravatar = setting_module.GetDefaultDisableGravatar()
disableGravatarSetting = &Setting{SettingValue: strconv.FormatBool(disableGravatar)}
} else if err != nil {
return err return err
} else {
disableGravatar = disableGravatarSetting.GetValueBool()
} }
var enableFederatedAvatar bool enableFederatedAvatar, err := GetSettingBool(ctx, KeyPictureEnableFederatedAvatar, setting_module.GetDefaultEnableFederatedAvatar(disableGravatar))
enableFederatedAvatarSetting, err := GetSetting(ctx, KeyPictureEnableFederatedAvatar) if err != nil {
if IsErrSettingIsNotExist(err) {
enableFederatedAvatar = setting_module.GetDefaultEnableFederatedAvatar(disableGravatar)
enableFederatedAvatarSetting = &Setting{SettingValue: strconv.FormatBool(enableFederatedAvatar)}
} else if err != nil {
return err return err
} else {
enableFederatedAvatar = disableGravatarSetting.GetValueBool()
} }
if setting_module.OfflineMode { if setting_module.OfflineMode {
disableGravatar = true if !disableGravatar {
enableFederatedAvatar = false
if !GetSettingBool(ctx, KeyPictureDisableGravatar) {
if err := SetSettingNoVersion(ctx, KeyPictureDisableGravatar, "true"); err != nil { if err := SetSettingNoVersion(ctx, KeyPictureDisableGravatar, "true"); err != nil {
return fmt.Errorf("Failed to set setting %q: %w", KeyPictureDisableGravatar, err) return fmt.Errorf("failed to set setting %q: %w", KeyPictureDisableGravatar, err)
} }
} }
if GetSettingBool(ctx, KeyPictureEnableFederatedAvatar) { disableGravatar = true
if enableFederatedAvatar {
if err := SetSettingNoVersion(ctx, KeyPictureEnableFederatedAvatar, "false"); err != nil { if err := SetSettingNoVersion(ctx, KeyPictureEnableFederatedAvatar, "false"); err != nil {
return fmt.Errorf("Failed to set setting %q: %w", KeyPictureEnableFederatedAvatar, err) return fmt.Errorf("failed to set setting %q: %w", KeyPictureEnableFederatedAvatar, err)
} }
} }
enableFederatedAvatar = false
} }
if enableFederatedAvatar || !disableGravatar { if enableFederatedAvatar || !disableGravatar {
var err error var err error
GravatarSourceURL, err = url.Parse(setting_module.GravatarSource) GravatarSourceURL, err = url.Parse(setting_module.GravatarSource)
if err != nil { if err != nil {
return fmt.Errorf("Failed to parse Gravatar URL(%s): %w", setting_module.GravatarSource, err) return fmt.Errorf("failed to parse Gravatar URL(%s): %w", setting_module.GravatarSource, err)
} }
} }
if GravatarSourceURL != nil && enableFederatedAvatarSetting.GetValueBool() { if GravatarSourceURL != nil && enableFederatedAvatar {
LibravatarService = libravatar.New() LibravatarService = libravatar.New()
if GravatarSourceURL.Scheme == "https" { if GravatarSourceURL.Scheme == "https" {
LibravatarService.SetUseHTTPS(true) LibravatarService.SetUseHTTPS(true)

View file

@ -67,7 +67,9 @@ func (u *User) AvatarLinkWithSize(ctx context.Context, size int) string {
useLocalAvatar := false useLocalAvatar := false
autoGenerateAvatar := false autoGenerateAvatar := false
disableGravatar := system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar) disableGravatar := system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar,
setting.GetDefaultDisableGravatar(),
)
switch { switch {
case u.UseCustomAvatar: case u.UseCustomAvatar:

View file

@ -11,6 +11,7 @@ import (
"code.gitea.io/gitea/models/avatars" "code.gitea.io/gitea/models/avatars"
user_model "code.gitea.io/gitea/models/user" user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/cache"
"code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/setting"
@ -34,42 +35,36 @@ type PushCommits struct {
HeadCommit *PushCommit HeadCommit *PushCommit
CompareURL string CompareURL string
Len int Len int
avatars map[string]string
emailUsers map[string]*user_model.User
} }
// NewPushCommits creates a new PushCommits object. // NewPushCommits creates a new PushCommits object.
func NewPushCommits() *PushCommits { func NewPushCommits() *PushCommits {
return &PushCommits{ return &PushCommits{}
avatars: make(map[string]string),
emailUsers: make(map[string]*user_model.User),
}
} }
// toAPIPayloadCommit converts a single PushCommit to an api.PayloadCommit object. // toAPIPayloadCommit converts a single PushCommit to an api.PayloadCommit object.
func (pc *PushCommits) toAPIPayloadCommit(ctx context.Context, repoPath, repoLink string, commit *PushCommit) (*api.PayloadCommit, error) { func (pc *PushCommits) toAPIPayloadCommit(ctx context.Context, emailUsers map[string]*user_model.User, repoPath, repoLink string, commit *PushCommit) (*api.PayloadCommit, error) {
var err error var err error
authorUsername := "" authorUsername := ""
author, ok := pc.emailUsers[commit.AuthorEmail] author, ok := emailUsers[commit.AuthorEmail]
if !ok { if !ok {
author, err = user_model.GetUserByEmail(ctx, commit.AuthorEmail) author, err = user_model.GetUserByEmail(ctx, commit.AuthorEmail)
if err == nil { if err == nil {
authorUsername = author.Name authorUsername = author.Name
pc.emailUsers[commit.AuthorEmail] = author emailUsers[commit.AuthorEmail] = author
} }
} else { } else {
authorUsername = author.Name authorUsername = author.Name
} }
committerUsername := "" committerUsername := ""
committer, ok := pc.emailUsers[commit.CommitterEmail] committer, ok := emailUsers[commit.CommitterEmail]
if !ok { if !ok {
committer, err = user_model.GetUserByEmail(ctx, commit.CommitterEmail) committer, err = user_model.GetUserByEmail(ctx, commit.CommitterEmail)
if err == nil { if err == nil {
// TODO: check errors other than email not found. // TODO: check errors other than email not found.
committerUsername = committer.Name committerUsername = committer.Name
pc.emailUsers[commit.CommitterEmail] = committer emailUsers[commit.CommitterEmail] = committer
} }
} else { } else {
committerUsername = committer.Name committerUsername = committer.Name
@ -107,11 +102,10 @@ func (pc *PushCommits) ToAPIPayloadCommits(ctx context.Context, repoPath, repoLi
commits := make([]*api.PayloadCommit, len(pc.Commits)) commits := make([]*api.PayloadCommit, len(pc.Commits))
var headCommit *api.PayloadCommit var headCommit *api.PayloadCommit
if pc.emailUsers == nil { emailUsers := make(map[string]*user_model.User)
pc.emailUsers = make(map[string]*user_model.User)
}
for i, commit := range pc.Commits { for i, commit := range pc.Commits {
apiCommit, err := pc.toAPIPayloadCommit(ctx, repoPath, repoLink, commit) apiCommit, err := pc.toAPIPayloadCommit(ctx, emailUsers, repoPath, repoLink, commit)
if err != nil { if err != nil {
return nil, nil, err return nil, nil, err
} }
@ -123,7 +117,7 @@ func (pc *PushCommits) ToAPIPayloadCommits(ctx context.Context, repoPath, repoLi
} }
if pc.HeadCommit != nil && headCommit == nil { if pc.HeadCommit != nil && headCommit == nil {
var err error var err error
headCommit, err = pc.toAPIPayloadCommit(ctx, repoPath, repoLink, pc.HeadCommit) headCommit, err = pc.toAPIPayloadCommit(ctx, emailUsers, repoPath, repoLink, pc.HeadCommit)
if err != nil { if err != nil {
return nil, nil, err return nil, nil, err
} }
@ -134,35 +128,21 @@ func (pc *PushCommits) ToAPIPayloadCommits(ctx context.Context, repoPath, repoLi
// AvatarLink tries to match user in database with e-mail // AvatarLink tries to match user in database with e-mail
// in order to show custom avatar, and falls back to general avatar link. // in order to show custom avatar, and falls back to general avatar link.
func (pc *PushCommits) AvatarLink(ctx context.Context, email string) string { func (pc *PushCommits) AvatarLink(ctx context.Context, email string) string {
if pc.avatars == nil {
pc.avatars = make(map[string]string)
}
avatar, ok := pc.avatars[email]
if ok {
return avatar
}
size := avatars.DefaultAvatarPixelSize * setting.Avatar.RenderedSizeFactor size := avatars.DefaultAvatarPixelSize * setting.Avatar.RenderedSizeFactor
u, ok := pc.emailUsers[email] v, _ := cache.GetWithContextCache(ctx, "push_commits", email, func() (string, error) {
if !ok { u, err := user_model.GetUserByEmail(ctx, email)
var err error
u, err = user_model.GetUserByEmail(ctx, email)
if err != nil { if err != nil {
pc.avatars[email] = avatars.GenerateEmailAvatarFastLink(ctx, email, size)
if !user_model.IsErrUserNotExist(err) { if !user_model.IsErrUserNotExist(err) {
log.Error("GetUserByEmail: %v", err) log.Error("GetUserByEmail: %v", err)
return "" return "", err
} }
} else { return avatars.GenerateEmailAvatarFastLink(ctx, email, size), nil
pc.emailUsers[email] = u
}
}
if u != nil {
pc.avatars[email] = u.AvatarLinkWithSize(ctx, size)
} }
return u.AvatarLinkWithSize(ctx, size), nil
})
return pc.avatars[email] return v
} }
// CommitToPushCommit transforms a git.Commit to PushCommit type. // CommitToPushCommit transforms a git.Commit to PushCommit type.
@ -189,7 +169,5 @@ func GitToPushCommits(gitCommits []*git.Commit) *PushCommits {
HeadCommit: nil, HeadCommit: nil,
CompareURL: "", CompareURL: "",
Len: len(commits), Len: len(commits),
avatars: make(map[string]string),
emailUsers: make(map[string]*user_model.User),
} }
} }

View file

@ -103,11 +103,9 @@ func TestPushCommits_ToAPIPayloadCommits(t *testing.T) {
assert.EqualValues(t, []string{"readme.md"}, headCommit.Modified) assert.EqualValues(t, []string{"readme.md"}, headCommit.Modified)
} }
func enableGravatar(t *testing.T) { func initGravatarSource(t *testing.T) {
err := system_model.SetSettingNoVersion(db.DefaultContext, system_model.KeyPictureDisableGravatar, "false")
assert.NoError(t, err)
setting.GravatarSource = "https://secure.gravatar.com/avatar" setting.GravatarSource = "https://secure.gravatar.com/avatar"
err = system_model.Init(db.DefaultContext) err := system_model.Init(db.DefaultContext)
assert.NoError(t, err) assert.NoError(t, err)
} }
@ -134,7 +132,7 @@ func TestPushCommits_AvatarLink(t *testing.T) {
}, },
} }
enableGravatar(t) initGravatarSource(t)
assert.Equal(t, assert.Equal(t,
"https://secure.gravatar.com/avatar/ab53a2911ddf9b4817ac01ddcd3d975f?d=identicon&s="+strconv.Itoa(28*setting.Avatar.RenderedSizeFactor), "https://secure.gravatar.com/avatar/ab53a2911ddf9b4817ac01ddcd3d975f?d=identicon&s="+strconv.Itoa(28*setting.Avatar.RenderedSizeFactor),

View file

@ -314,7 +314,9 @@ func EditUser(ctx *context.Context) {
ctx.Data["DisableRegularOrgCreation"] = setting.Admin.DisableRegularOrgCreation ctx.Data["DisableRegularOrgCreation"] = setting.Admin.DisableRegularOrgCreation
ctx.Data["DisableMigrations"] = setting.Repository.DisableMigrations ctx.Data["DisableMigrations"] = setting.Repository.DisableMigrations
ctx.Data["AllowedUserVisibilityModes"] = setting.Service.AllowedUserVisibilityModesSlice.ToVisibleTypeSlice() ctx.Data["AllowedUserVisibilityModes"] = setting.Service.AllowedUserVisibilityModesSlice.ToVisibleTypeSlice()
ctx.Data["DisableGravatar"] = system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar) ctx.Data["DisableGravatar"] = system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar,
setting.GetDefaultDisableGravatar(),
)
prepareUserInfo(ctx) prepareUserInfo(ctx)
if ctx.Written() { if ctx.Written() {
@ -331,7 +333,8 @@ func EditUserPost(ctx *context.Context) {
ctx.Data["PageIsAdminUsers"] = true ctx.Data["PageIsAdminUsers"] = true
ctx.Data["DisableMigrations"] = setting.Repository.DisableMigrations ctx.Data["DisableMigrations"] = setting.Repository.DisableMigrations
ctx.Data["AllowedUserVisibilityModes"] = setting.Service.AllowedUserVisibilityModesSlice.ToVisibleTypeSlice() ctx.Data["AllowedUserVisibilityModes"] = setting.Service.AllowedUserVisibilityModesSlice.ToVisibleTypeSlice()
ctx.Data["DisableGravatar"] = system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar) ctx.Data["DisableGravatar"] = system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar,
setting.GetDefaultDisableGravatar())
u := prepareUserInfo(ctx) u := prepareUserInfo(ctx)
if ctx.Written() { if ctx.Written() {

View file

@ -44,7 +44,9 @@ func Profile(ctx *context.Context) {
ctx.Data["Title"] = ctx.Tr("settings.profile") ctx.Data["Title"] = ctx.Tr("settings.profile")
ctx.Data["PageIsSettingsProfile"] = true ctx.Data["PageIsSettingsProfile"] = true
ctx.Data["AllowedUserVisibilityModes"] = setting.Service.AllowedUserVisibilityModesSlice.ToVisibleTypeSlice() ctx.Data["AllowedUserVisibilityModes"] = setting.Service.AllowedUserVisibilityModesSlice.ToVisibleTypeSlice()
ctx.Data["DisableGravatar"] = system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar) ctx.Data["DisableGravatar"] = system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar,
setting.GetDefaultDisableGravatar(),
)
ctx.HTML(http.StatusOK, tplSettingsProfile) ctx.HTML(http.StatusOK, tplSettingsProfile)
} }
@ -86,7 +88,9 @@ func ProfilePost(ctx *context.Context) {
ctx.Data["Title"] = ctx.Tr("settings") ctx.Data["Title"] = ctx.Tr("settings")
ctx.Data["PageIsSettingsProfile"] = true ctx.Data["PageIsSettingsProfile"] = true
ctx.Data["AllowedUserVisibilityModes"] = setting.Service.AllowedUserVisibilityModesSlice.ToVisibleTypeSlice() ctx.Data["AllowedUserVisibilityModes"] = setting.Service.AllowedUserVisibilityModesSlice.ToVisibleTypeSlice()
ctx.Data["DisableGravatar"] = system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar) ctx.Data["DisableGravatar"] = system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar,
setting.GetDefaultDisableGravatar(),
)
if ctx.HasError() { if ctx.HasError() {
ctx.HTML(http.StatusOK, tplSettingsProfile) ctx.HTML(http.StatusOK, tplSettingsProfile)