Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add context cache as a request level cache #22294

Merged
merged 27 commits into from
Feb 15, 2023
Merged

Add context cache as a request level cache #22294

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
901cdb2
Add context cache support
lunny Dec 31, 2022
94a7ddf
Fix test
lunny Dec 31, 2022
1c89c15
Merge branch 'main' into lunny/context_cache
lunny Dec 31, 2022
109979d
replace template function
lunny Jan 1, 2023
96b7e2d
Fix bug
lunny Jan 18, 2023
e455e8b
Fix bug
lunny Jan 18, 2023
4a2f3fe
Fix bug
lunny Jan 19, 2023
f1e7b34
Merge branch 'main' into lunny/context_cache
lunny Jan 19, 2023
1f68f88
Fix bug
lunny Jan 19, 2023
4727355
Fix bug
lunny Jan 19, 2023
89600f7
Add unit test
lunny Jan 19, 2023
696e0d3
merge main branch
lunny Jan 21, 2023
b0f0267
merge main branch
lunny Jan 29, 2023
728ae6c
Fix multiple goroutines problem
lunny Feb 4, 2023
57187ef
merge main branch
lunny Feb 4, 2023
35221d6
Merge branch 'main' into lunny/context_cache
lunny Feb 4, 2023
5a1fad6
a new generic function
lunny Feb 5, 2023
8a3294e
Merge branch 'main' into lunny/context_cache
lunny Feb 5, 2023
bb279f6
make context cache data private
lunny Feb 6, 2023
296b9c2
Merge branch 'main' into lunny/context_cache
lunny Feb 6, 2023
3f2869d
some slight changes
lunny Feb 6, 2023
d25b6b5
Add some comment and test
lunny Feb 6, 2023
a98791c
merge main branch
lunny Feb 7, 2023
3b26a55
Fix template wrong ref
lunny Feb 9, 2023
95375f7
Merge branch 'main' into lunny/context_cache
lunny Feb 9, 2023
669d5c3
merge main branch
lunny Feb 14, 2023
1474785
merge main branch
lunny Feb 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix test
  • Loading branch information
@lunny
lunny committed Dec 31, 2022
commit 94a7ddf747aad6acefe706664dbc0a90255e4eca
11 changes: 6 additions & 5 deletions models/avatars/avatar_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"testing"

avatars_model "code.gitea.io/gitea/models/avatars"
"code.gitea.io/gitea/models/db"
system_model "code.gitea.io/gitea/models/system"
"code.gitea.io/gitea/modules/setting"

Expand All @@ -16,15 +17,15 @@ import (
const gravatarSource = "https://secure.gravatar.com/avatar/"

func disableGravatar(t *testing.T) {
err := system_model.SetSettingNoVersion(system_model.KeyPictureEnableFederatedAvatar, "false")
err := system_model.SetSettingNoVersion(db.DefaultContext, system_model.KeyPictureEnableFederatedAvatar, "false")
assert.NoError(t, err)
err = system_model.SetSettingNoVersion(system_model.KeyPictureDisableGravatar, "true")
err = system_model.SetSettingNoVersion(db.DefaultContext, system_model.KeyPictureDisableGravatar, "true")
assert.NoError(t, err)
system_model.LibravatarService = nil
}

func enableGravatar(t *testing.T) {
err := system_model.SetSettingNoVersion(system_model.KeyPictureDisableGravatar, "false")
err := system_model.SetSettingNoVersion(db.DefaultContext, system_model.KeyPictureDisableGravatar, "false")
assert.NoError(t, err)
setting.GravatarSource = gravatarSource
err = system_model.Init()
Expand All @@ -47,11 +48,11 @@ func TestSizedAvatarLink(t *testing.T) {

disableGravatar(t)
assert.Equal(t, "/testsuburl/assets/img/avatar_default.png",
avatars_model.GenerateEmailAvatarFastLink("gitea@example.com", 100))
avatars_model.GenerateEmailAvatarFastLink(db.DefaultContext, "gitea@example.com", 100))

enableGravatar(t)
assert.Equal(t,
"https://secure.gravatar.com/avatar/353cbad9b58e69c96154ad99f92bedc7?d=identicon&s=100",
avatars_model.GenerateEmailAvatarFastLink("gitea@example.com", 100),
avatars_model.GenerateEmailAvatarFastLink(db.DefaultContext, "gitea@example.com", 100),
)
}
8 changes: 6 additions & 2 deletions models/system/setting.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,13 @@ func GetSettings(ctx context.Context, keys []string) (map[string]*Setting, error
newKeys = append(newKeys, key)
}
}
settings := make([]*Setting, 0, len(keys))
if len(newKeys) == 0 {
return settingsMap, nil
}

settings := make([]*Setting, 0, len(newKeys))
if err := db.GetEngine(db.DefaultContext).
Where(builder.In("setting_key", keys)).
Where(builder.In("setting_key", newKeys)).
Find(&settings); err != nil {
return nil, err
}
Expand Down
11 changes: 6 additions & 5 deletions models/system/setting_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"strings"
"testing"

"code.gitea.io/gitea/models/db"
"code.gitea.io/gitea/models/system"
"code.gitea.io/gitea/models/unittest"

Expand All @@ -20,21 +21,21 @@ func TestSettings(t *testing.T) {
newSetting := &system.Setting{SettingKey: keyName, SettingValue: "50"}

// create setting
err := system.SetSetting(newSetting)
err := system.SetSetting(db.DefaultContext, newSetting)
assert.NoError(t, err)
// test about saving unchanged values
err = system.SetSetting(newSetting)
err = system.SetSetting(db.DefaultContext, newSetting)
assert.NoError(t, err)

// get specific setting
settings, err := system.GetSettings([]string{keyName})
settings, err := system.GetSettings(db.DefaultContext, []string{keyName})
assert.NoError(t, err)
assert.Len(t, settings, 1)
assert.EqualValues(t, newSetting.SettingValue, settings[strings.ToLower(keyName)].SettingValue)

// updated setting
updatedSetting := &system.Setting{SettingKey: keyName, SettingValue: "100", Version: newSetting.Version}
err = system.SetSetting(updatedSetting)
err = system.SetSetting(db.DefaultContext, updatedSetting)
assert.NoError(t, err)

// get all settings
Expand All @@ -44,7 +45,7 @@ func TestSettings(t *testing.T) {
assert.EqualValues(t, updatedSetting.SettingValue, settings[strings.ToLower(updatedSetting.SettingKey)].SettingValue)

// delete setting
err = system.DeleteSetting(&system.Setting{SettingKey: strings.ToLower(keyName)})
err = system.DeleteSetting(db.DefaultContext, &system.Setting{SettingKey: strings.ToLower(keyName)})
assert.NoError(t, err)
settings, err = system.GetAllSettings()
assert.NoError(t, err)
Expand Down
12 changes: 6 additions & 6 deletions modules/cache/context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,15 @@ package cache

import "context"

type CacheContext struct {
type cacheContext struct {
ctx context.Context
Data map[any]map[any]any
}

type CacheContextKey struct{}
type cacheContextKey struct{}

func WithCacheContext(ctx context.Context) context.Context {
return context.WithValue(ctx, &CacheContextKey{}, &CacheContext{
return context.WithValue(ctx, &cacheContextKey{}, &cacheContext{
ctx: ctx,
Data: make(map[any]map[any]any),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a feeling that making this an unlimited map instead of something limited in size will bite us as more and more things use to use the cache.

Whilst LRUs have an overhead it might be a better idea to make this a generous LRU.

I guess it might be sensible to provide a few implementations of a context cache (LRU/hard limit map/no limit map) here, AND/OR to add some trace logging/pprof metrics that counts the number of entities that gets stored in these maps.

As an aside the complicated changes within models/settings do show the failure of the current design of our db services - these probably need to be migrated to service interfaces which would allow for simpler cache wrapping interfaces.

Copy link
Member Author

@lunny lunny Feb 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a request, I cannot imagine it's an unlimited map. And in fact, all the values are pointers, only keys are in this map. When a request is end, the map and related objects will be GC.

})
Expand All @@ -23,7 +23,7 @@ func GetContextData(ctx context.Context, tp, key any) any {
if ctx == nil {
return nil
}
if c, ok := ctx.Value(&CacheContextKey{}).(*CacheContext); ok {
if c, ok := ctx.Value(&cacheContextKey{}).(*cacheContext); ok {
if c.Data[tp] != nil {
return c.Data[tp][key]
}
Expand All @@ -35,7 +35,7 @@ func SetContextData(ctx context.Context, tp, key, value any) {
if ctx == nil {
return
}
if c, ok := ctx.Value(&CacheContextKey{}).(*CacheContext); ok {
if c, ok := ctx.Value(&cacheContextKey{}).(*cacheContext); ok {
if c.Data[tp] == nil {
c.Data[tp] = make(map[any]any)
}
Expand All @@ -47,7 +47,7 @@ func RemoveContextData(ctx context.Context, tp, key any) {
if ctx == nil {
return
}
if c, ok := ctx.Value(&CacheContextKey{}).(*CacheContext); ok {
if c, ok := ctx.Value(&cacheContextKey{}).(*cacheContext); ok {
if c.Data[tp] == nil {
c.Data[tp] = make(map[any]any)
}
Expand Down
7 changes: 4 additions & 3 deletions modules/repository/commits_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"testing"
"time"

"code.gitea.io/gitea/models/db"
repo_model "code.gitea.io/gitea/models/repo"
system_model "code.gitea.io/gitea/models/system"
"code.gitea.io/gitea/models/unittest"
Expand Down Expand Up @@ -102,7 +103,7 @@ func TestPushCommits_ToAPIPayloadCommits(t *testing.T) {
}

func enableGravatar(t *testing.T) {
err := system_model.SetSettingNoVersion(system_model.KeyPictureDisableGravatar, "false")
err := system_model.SetSettingNoVersion(db.DefaultContext, system_model.KeyPictureDisableGravatar, "false")
assert.NoError(t, err)
setting.GravatarSource = "https://secure.gravatar.com/avatar"
err = system_model.Init()
Expand Down Expand Up @@ -136,13 +137,13 @@ func TestPushCommits_AvatarLink(t *testing.T) {

assert.Equal(t,
"https://secure.gravatar.com/avatar/ab53a2911ddf9b4817ac01ddcd3d975f?d=identicon&s=84",
pushCommits.AvatarLink("user2@example.com"))
pushCommits.AvatarLink(db.DefaultContext, "user2@example.com"))

assert.Equal(t,
"https://secure.gravatar.com/avatar/"+
fmt.Sprintf("%x", md5.Sum([]byte("nonexistent@example.com")))+
"?d=identicon&s=84",
pushCommits.AvatarLink("nonexistent@example.com"))
pushCommits.AvatarLink(db.DefaultContext, "nonexistent@example.com"))
}

func TestCommitToPushCommit(t *testing.T) {
Expand Down
3 changes: 2 additions & 1 deletion routers/common/middleware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"net/http"
"strings"

"code.gitea.io/gitea/modules/cache"
"code.gitea.io/gitea/modules/context"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/process"
Expand All @@ -28,7 +29,7 @@ func Middlewares() []func(http.Handler) http.Handler {

ctx, _, finished := process.GetManager().AddTypedContext(req.Context(), fmt.Sprintf("%s: %s", req.Method, req.RequestURI), process.RequestProcessType, true)
defer finished()
next.ServeHTTP(context.NewResponse(resp), req.WithContext(ctx))
next.ServeHTTP(context.NewResponse(resp), req.WithContext(cache.WithCacheContext(ctx)))
})
},
}
Expand Down
4 changes: 2 additions & 2 deletions routers/web/auth/oauth_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ func TestNewAccessTokenResponse_OIDCToken(t *testing.T) {
assert.Equal(t, user.Name, oidcToken.Name)
assert.Equal(t, user.Name, oidcToken.PreferredUsername)
assert.Equal(t, user.HTMLURL(), oidcToken.Profile)
assert.Equal(t, user.AvatarLink(), oidcToken.Picture)
assert.Equal(t, user.AvatarLink(db.DefaultContext), oidcToken.Picture)
assert.Equal(t, user.Website, oidcToken.Website)
assert.Equal(t, user.UpdatedUnix, oidcToken.UpdatedAt)
assert.Equal(t, user.Email, oidcToken.Email)
Expand All @@ -87,7 +87,7 @@ func TestNewAccessTokenResponse_OIDCToken(t *testing.T) {
assert.Equal(t, user.FullName, oidcToken.Name)
assert.Equal(t, user.Name, oidcToken.PreferredUsername)
assert.Equal(t, user.HTMLURL(), oidcToken.Profile)
assert.Equal(t, user.AvatarLink(), oidcToken.Picture)
assert.Equal(t, user.AvatarLink(db.DefaultContext), oidcToken.Picture)
assert.Equal(t, user.Website, oidcToken.Website)
assert.Equal(t, user.UpdatedUnix, oidcToken.UpdatedAt)
assert.Equal(t, user.Email, oidcToken.Email)
Expand Down
9 changes: 5 additions & 4 deletions services/convert/user_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package convert
import (
"testing"

"code.gitea.io/gitea/models/db"
"code.gitea.io/gitea/models/unittest"
user_model "code.gitea.io/gitea/models/user"
api "code.gitea.io/gitea/modules/structs"
Expand All @@ -18,22 +19,22 @@ func TestUser_ToUser(t *testing.T) {

user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1, IsAdmin: true})

apiUser := toUser(user1, true, true)
apiUser := toUser(db.DefaultContext, user1, true, true)
assert.True(t, apiUser.IsAdmin)
assert.Contains(t, apiUser.AvatarURL, "://")

user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2, IsAdmin: false})

apiUser = toUser(user2, true, true)
apiUser = toUser(db.DefaultContext, user2, true, true)
assert.False(t, apiUser.IsAdmin)

apiUser = toUser(user1, false, false)
apiUser = toUser(db.DefaultContext, user1, false, false)
assert.False(t, apiUser.IsAdmin)
assert.EqualValues(t, api.VisibleTypePublic.String(), apiUser.Visibility)

user31 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 31, IsAdmin: false, Visibility: api.VisibleTypePrivate})

apiUser = toUser(user31, true, true)
apiUser = toUser(db.DefaultContext, user31, true, true)
assert.False(t, apiUser.IsAdmin)
assert.EqualValues(t, api.VisibleTypePrivate.String(), apiUser.Visibility)
}
2 changes: 1 addition & 1 deletion services/user/user_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ func TestCreateUser_Issue5882(t *testing.T) {

assert.NoError(t, user_model.CreateUser(v.user))

u, err := user_model.GetUserByEmail(v.user.Email)
u, err := user_model.GetUserByEmail(db.DefaultContext, v.user.Email)
assert.NoError(t, err)

assert.Equal(t, !u.AllowCreateOrganization, v.disableOrgCreation)
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/api_comment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ func TestAPIGetComment(t *testing.T) {
DecodeJSON(t, resp, &apiComment)

assert.NoError(t, comment.LoadPoster(db.DefaultContext))
expect := convert.ToComment(comment)
expect := convert.ToComment(db.DefaultContext, comment)

assert.Equal(t, expect.ID, apiComment.ID)
assert.Equal(t, expect.Poster.FullName, apiComment.Poster.FullName)
Expand Down
6 changes: 3 additions & 3 deletions tests/integration/api_issue_reaction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func TestAPIIssuesReactions(t *testing.T) {
DecodeJSON(t, resp, &apiReactions)
expectResponse := make(map[int]api.Reaction)
expectResponse[0] = api.Reaction{
User: convert.ToUser(user2, user2),
User: convert.ToUser(db.DefaultContext, user2, user2),
Reaction: "eyes",
Created: time.Unix(1573248003, 0),
}
Expand Down Expand Up @@ -124,12 +124,12 @@ func TestAPICommentReactions(t *testing.T) {
DecodeJSON(t, resp, &apiReactions)
expectResponse := make(map[int]api.Reaction)
expectResponse[0] = api.Reaction{
User: convert.ToUser(user2, user2),
User: convert.ToUser(db.DefaultContext, user2, user2),
Reaction: "laugh",
Created: time.Unix(1573248004, 0),
}
expectResponse[1] = api.Reaction{
User: convert.ToUser(user1, user1),
User: convert.ToUser(db.DefaultContext, user1, user1),
Reaction: "laugh",
Created: time.Unix(1573248005, 0),
}
Expand Down
3 changes: 2 additions & 1 deletion tests/integration/api_team_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"sort"
"testing"

"code.gitea.io/gitea/models/db"
"code.gitea.io/gitea/models/organization"
"code.gitea.io/gitea/models/repo"
"code.gitea.io/gitea/models/unit"
Expand Down Expand Up @@ -209,7 +210,7 @@ func checkTeamResponse(t *testing.T, testName string, apiTeam *api.Team, name, d
func checkTeamBean(t *testing.T, id int64, name, description string, includesAllRepositories bool, permission string, units []string, unitsMap map[string]string) {
team := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: id})
assert.NoError(t, team.GetUnits(), "GetUnits")
apiTeam, err := convert.ToTeam(team)
apiTeam, err := convert.ToTeam(db.DefaultContext, team)
assert.NoError(t, err)
checkTeamResponse(t, fmt.Sprintf("checkTeamBean/%s_%s", name, description), apiTeam, name, description, includesAllRepositories, permission, units, unitsMap)
}
Expand Down
3 changes: 2 additions & 1 deletion tests/integration/api_team_user_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"testing"
"time"

"code.gitea.io/gitea/models/db"
"code.gitea.io/gitea/models/unittest"
user_model "code.gitea.io/gitea/models/user"
api "code.gitea.io/gitea/modules/structs"
Expand All @@ -33,7 +34,7 @@ func TestAPITeamUser(t *testing.T) {
user2.Created = user2.Created.In(time.Local)
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user2"})

expectedUser := convert.ToUser(user, user)
expectedUser := convert.ToUser(db.DefaultContext, user, user)

// test time via unix timestamp
assert.EqualValues(t, expectedUser.LastLogin.Unix(), user2.LastLogin.Unix())
Expand Down
9 changes: 5 additions & 4 deletions tests/integration/api_user_orgs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"net/http"
"testing"

"code.gitea.io/gitea/models/db"
"code.gitea.io/gitea/models/unittest"
user_model "code.gitea.io/gitea/models/user"
api "code.gitea.io/gitea/modules/structs"
Expand All @@ -34,7 +35,7 @@ func TestUserOrgs(t *testing.T) {
Name: user17.Name,
UserName: user17.Name,
FullName: user17.FullName,
AvatarURL: user17.AvatarLink(),
AvatarURL: user17.AvatarLink(db.DefaultContext),
Description: "",
Website: "",
Location: "",
Expand All @@ -45,7 +46,7 @@ func TestUserOrgs(t *testing.T) {
Name: user3.Name,
UserName: user3.Name,
FullName: user3.FullName,
AvatarURL: user3.AvatarLink(),
AvatarURL: user3.AvatarLink(db.DefaultContext),
Description: "",
Website: "",
Location: "",
Expand Down Expand Up @@ -99,7 +100,7 @@ func TestMyOrgs(t *testing.T) {
Name: user17.Name,
UserName: user17.Name,
FullName: user17.FullName,
AvatarURL: user17.AvatarLink(),
AvatarURL: user17.AvatarLink(db.DefaultContext),
Description: "",
Website: "",
Location: "",
Expand All @@ -110,7 +111,7 @@ func TestMyOrgs(t *testing.T) {
Name: user3.Name,
UserName: user3.Name,
FullName: user3.FullName,
AvatarURL: user3.AvatarLink(),
AvatarURL: user3.AvatarLink(db.DefaultContext),
Description: "",
Website: "",
Location: "",
Expand Down
3 changes: 2 additions & 1 deletion tests/integration/user_avatar_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"net/url"
"testing"

"code.gitea.io/gitea/models/db"
"code.gitea.io/gitea/models/unittest"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/avatar"
Expand Down Expand Up @@ -73,7 +74,7 @@ func TestUserAvatar(t *testing.T) {

user2 = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) // owner of the repo3, is an org

req = NewRequest(t, "GET", user2.AvatarLinkWithSize(0))
req = NewRequest(t, "GET", user2.AvatarLinkWithSize(db.DefaultContext, 0))
_ = session.MakeRequest(t, req, http.StatusOK)

// Can't test if the response matches because the image is re-generated on upload but checking that this at least doesn't give a 404 should be enough.
Expand Down