From 5979ce7823813b0a400f757ddb7515890b10c6ec Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Thu, 20 Aug 2020 11:29:25 +0530 Subject: [PATCH] MM-27883 - remove deep copies from the cache layer (#15271) When we are reading from or putting to the cache, there is no need to deep copy objects now because the cache is already serialized. We also delete some lines from tests because the mock store directly returns the pointers whereas actually they would be returned from the database where a serialization occurs again. So we would be testing for the wrong thing by unnecessarily keeping the deep copies for DB reads. https://mattermost.atlassian.net/browse/MM-27883 Co-authored-by: Mattermod --- store/localcachelayer/channel_layer.go | 3 +-- store/localcachelayer/user_layer.go | 25 +++++++----------------- store/localcachelayer/user_layer_test.go | 10 ---------- 3 files changed, 8 insertions(+), 30 deletions(-) diff --git a/store/localcachelayer/channel_layer.go b/store/localcachelayer/channel_layer.go index 32e72c6810..3e7a505033 100644 --- a/store/localcachelayer/channel_layer.go +++ b/store/localcachelayer/channel_layer.go @@ -159,8 +159,7 @@ func (s LocalCacheChannelStore) Get(id string, allowFromCache bool) (*model.Chan if allowFromCache { var cacheItem *model.Channel if err := s.rootStore.doStandardReadCache(s.rootStore.channelByIdCache, id, &cacheItem); err == nil { - ch := cacheItem.DeepCopy() - return ch, nil + return cacheItem, nil } } diff --git a/store/localcachelayer/user_layer.go b/store/localcachelayer/user_layer.go index c437296e2e..652e6e10b3 100644 --- a/store/localcachelayer/user_layer.go +++ b/store/localcachelayer/user_layer.go @@ -75,7 +75,7 @@ func (s LocalCacheUserStore) GetAllProfilesInChannel(channelId string, allowFrom if allowFromCache { var cachedMap map[string]*model.User if err := s.rootStore.doStandardReadCache(s.rootStore.profilesInChannelCache, channelId, &cachedMap); err == nil { - return deepCopyUserMap(cachedMap), nil + return cachedMap, nil } } @@ -85,7 +85,7 @@ func (s LocalCacheUserStore) GetAllProfilesInChannel(channelId string, allowFrom } if allowFromCache { - s.rootStore.doStandardAddToCache(s.rootStore.profilesInChannelCache, channelId, deepCopyUserMap(userMap)) + s.rootStore.doStandardAddToCache(s.rootStore.profilesInChannelCache, channelId, userMap) } return userMap, nil @@ -107,7 +107,7 @@ func (s LocalCacheUserStore) GetProfileByIds(userIds []string, options *store.Us var cacheItem *model.User if err := s.rootStore.doStandardReadCache(s.rootStore.userProfileByIdsCache, userId, &cacheItem); err == nil { if options.Since == 0 || cacheItem.UpdateAt > options.Since { - users = append(users, cacheItem.DeepCopy()) + users = append(users, cacheItem) } } else { remainingUserIds = append(remainingUserIds, userId) @@ -126,7 +126,7 @@ func (s LocalCacheUserStore) GetProfileByIds(userIds []string, options *store.Us } for _, user := range remainingUsers { s.rootStore.doStandardAddToCache(s.rootStore.userProfileByIdsCache, user.Id, user) - users = append(users, user.DeepCopy()) + users = append(users, user) } } @@ -143,7 +143,7 @@ func (s LocalCacheUserStore) Get(id string) (*model.User, *model.AppError) { if s.rootStore.metrics != nil { s.rootStore.metrics.AddMemCacheHitCounter("Profile By Id", float64(1)) } - return cacheItem.DeepCopy(), nil + return cacheItem, nil } if s.rootStore.metrics != nil { s.rootStore.metrics.AddMemCacheMissCounter("Profile By Id", float64(1)) @@ -152,17 +152,6 @@ func (s LocalCacheUserStore) Get(id string) (*model.User, *model.AppError) { if err != nil { return nil, err } - u := user.DeepCopy() - s.rootStore.doStandardAddToCache(s.rootStore.userProfileByIdsCache, id, u) - return user.DeepCopy(), nil -} - -func deepCopyUserMap(users map[string]*model.User) map[string]*model.User { - copyOfUsers := make(map[string]*model.User) - - for id, user := range users { - copyOfUsers[id] = user.DeepCopy() - } - - return copyOfUsers + s.rootStore.doStandardAddToCache(s.rootStore.userProfileByIdsCache, id, user) + return user, nil } diff --git a/store/localcachelayer/user_layer_test.go b/store/localcachelayer/user_layer_test.go index 1511832599..74b28233b6 100644 --- a/store/localcachelayer/user_layer_test.go +++ b/store/localcachelayer/user_layer_test.go @@ -91,11 +91,6 @@ func TestUserStoreCache(t *testing.T) { for i := 0; i < len(storedUsers); i++ { assert.Equal(t, storedUsers[i].Id, cachedUsers[i].Id) - if storedUsers[i] == cachedUsers[i] { - assert.Fail(t, "should be different pointers") - } - cachedUsers[i].NotifyProps["key"] = "othervalue" - assert.NotEqual(t, storedUsers[i], cachedUsers[i]) } cachedUsers, err = cachedStore.User().GetProfileByIds(fakeUserIds, &store.UserGetByIdsOpts{}, true) @@ -237,11 +232,6 @@ func TestUserStoreGetCache(t *testing.T) { cachedUser, err := cachedStore.User().Get(fakeUserId) require.Nil(t, err) assert.Equal(t, storedUser, cachedUser) - if storedUser == cachedUser { - assert.Fail(t, "should be different pointers") - } - cachedUser.NotifyProps["key"] = "othervalue" - assert.NotEqual(t, storedUser, cachedUser) storedUser.Props = model.StringMap{} storedUser.Timezone = model.StringMap{}