From 3c31629813b636d182e605e9947bfe5bc5b310bd Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Tue, 18 Jul 2023 20:27:48 +0530 Subject: [PATCH] MM-53669: Use the cache layer for EmojiStore.GetMultipleByName (#24030) There was already a cache present for emoji names. But we weren't using it for the GetMultipleByName method. Now we implement that method to look up the cache for every emoji name passed. Secondly, a bigger problem was that we were making the DB call for system emojis as well. Since system emojis aren't stored in the DB, it would fall through the cache layer and always make a redundant DB call. In the profiles, this should up as taking 16% of the total time to serve a getPostsForChannel API endpoint. We fix this by filtering the emojis to only custom emojis before making the call. https://mattermost.atlassian.net/browse/MM-53669 ```release-note NONE ``` --- server/channels/app/emoji.go | 22 ++++++++-- server/channels/app/emoji_test.go | 29 ++++++++++++ server/channels/app/post_metadata.go | 1 - .../store/localcachelayer/emoji_layer.go | 42 +++++++++++++++++- .../store/localcachelayer/emoji_layer_test.go | 44 ++++++++++++++++++- .../store/localcachelayer/main_test.go | 3 ++ .../opentracinglayer/opentracinglayer.go | 4 +- .../channels/store/retrylayer/retrylayer.go | 4 +- server/channels/store/sqlstore/emoji_store.go | 4 +- server/channels/store/store.go | 2 +- .../channels/store/storetest/emoji_store.go | 8 ++-- .../store/storetest/mocks/EmojiStore.go | 18 ++++---- .../channels/store/timerlayer/timerlayer.go | 4 +- 13 files changed, 154 insertions(+), 31 deletions(-) create mode 100644 server/channels/app/emoji_test.go diff --git a/server/channels/app/emoji.go b/server/channels/app/emoji.go index 73924114e8..273282181d 100644 --- a/server/channels/app/emoji.go +++ b/server/channels/app/emoji.go @@ -191,7 +191,7 @@ func (a *App) GetEmoji(c request.CTX, emojiId string) (*model.Emoji, *model.AppE return nil, model.NewAppError("GetEmoji", "api.emoji.storage.app_error", nil, "", http.StatusForbidden) } - emoji, err := a.Srv().Store().Emoji().Get(context.Background(), emojiId, true) + emoji, err := a.Srv().Store().Emoji().Get(c.Context(), emojiId, true) if err != nil { var nfErr *store.ErrNotFound switch { @@ -214,7 +214,7 @@ func (a *App) GetEmojiByName(c request.CTX, emojiName string) (*model.Emoji, *mo return nil, model.NewAppError("GetEmojiByName", "api.emoji.storage.app_error", nil, "", http.StatusForbidden) } - emoji, err := a.Srv().Store().Emoji().GetByName(context.Background(), emojiName, true) + emoji, err := a.Srv().Store().Emoji().GetByName(c.Context(), emojiName, true) if err != nil { var nfErr *store.ErrNotFound switch { @@ -233,7 +233,21 @@ func (a *App) GetMultipleEmojiByName(c request.CTX, names []string) ([]*model.Em return nil, model.NewAppError("GetMultipleEmojiByName", "api.emoji.disabled.app_error", nil, "", http.StatusForbidden) } - emoji, err := a.Srv().Store().Emoji().GetMultipleByName(names) + // Filtering out system emojis + i := 0 + for _, n := range names { + if _, ok := model.GetSystemEmojiId(n); !ok { + names[i] = n + i++ + } + } + names = names[:i] + + if len(names) == 0 { + return []*model.Emoji{}, nil + } + + emoji, err := a.Srv().Store().Emoji().GetMultipleByName(c.Context(), names) if err != nil { return nil, model.NewAppError("GetMultipleEmojiByName", "app.emoji.get_by_name.app_error", nil, fmt.Sprintf("names=%v, %v", names, err.Error()), http.StatusInternalServerError) } @@ -242,7 +256,7 @@ func (a *App) GetMultipleEmojiByName(c request.CTX, names []string) ([]*model.Em } func (a *App) GetEmojiImage(c request.CTX, emojiId string) ([]byte, string, *model.AppError) { - _, storeErr := a.Srv().Store().Emoji().Get(context.Background(), emojiId, true) + _, storeErr := a.Srv().Store().Emoji().Get(c.Context(), emojiId, true) if storeErr != nil { var nfErr *store.ErrNotFound switch { diff --git a/server/channels/app/emoji_test.go b/server/channels/app/emoji_test.go new file mode 100644 index 0000000000..dfaacc123c --- /dev/null +++ b/server/channels/app/emoji_test.go @@ -0,0 +1,29 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package app + +import ( + "testing" + + "github.com/mattermost/mattermost/server/public/model" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGetMultipleEmojiByName(t *testing.T) { + // The fact that we use mock store ensures that + // the call to the DB does not happen. If it did, we would have needed + // to provide the mock explicitly. + th := SetupWithStoreMock(t) + defer th.TearDown() + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.EnableCustomEmoji = true + }) + + // Ensure it returns empty for system emojis + emojis, appErr := th.App.GetMultipleEmojiByName(th.Context, []string{"+1"}) + require.Nil(t, appErr) + assert.Empty(t, emojis) +} diff --git a/server/channels/app/post_metadata.go b/server/channels/app/post_metadata.go index 55a555d96c..94ad1b0dfa 100644 --- a/server/channels/app/post_metadata.go +++ b/server/channels/app/post_metadata.go @@ -438,7 +438,6 @@ func (a *App) getCustomEmojisForPost(c request.CTX, post *model.Post, reactions } names := getEmojiNamesForPost(post, reactions) - if len(names) == 0 { return []*model.Emoji{}, nil } diff --git a/server/channels/store/localcachelayer/emoji_layer.go b/server/channels/store/localcachelayer/emoji_layer.go index 93039de03c..cb5b4659c6 100644 --- a/server/channels/store/localcachelayer/emoji_layer.go +++ b/server/channels/store/localcachelayer/emoji_layer.go @@ -90,12 +90,50 @@ func (es *LocalCacheEmojiStore) GetByName(ctx context.Context, name string, allo es.emojiByNameMut.Unlock() emoji, err := es.EmojiStore.GetByName(ctx, name, allowFromCache) + if err != nil { + return nil, err + } - if allowFromCache && err == nil { + if allowFromCache { es.addToCache(emoji) } - return emoji, err + return emoji, nil +} + +func (es *LocalCacheEmojiStore) GetMultipleByName(ctx context.Context, names []string) ([]*model.Emoji, error) { + emojis := []*model.Emoji{} + remainingEmojiNames := make([]string, 0) + + for _, name := range names { + if emoji, ok := es.getFromCacheByName(name); ok { + emojis = append(emojis, emoji) + } else { + // If it was invalidated, then we need to query master. + es.emojiByNameMut.Lock() + if es.emojiByNameInvalidations[name] { + ctx = sqlstore.WithMaster(ctx) + // And then remove the key from the map. + delete(es.emojiByNameInvalidations, name) + } + es.emojiByNameMut.Unlock() + + remainingEmojiNames = append(remainingEmojiNames, name) + } + } + + if len(remainingEmojiNames) > 0 { + remainingEmojis, err := es.EmojiStore.GetMultipleByName(ctx, remainingEmojiNames) + if err != nil { + return nil, err + } + for _, emoji := range remainingEmojis { + es.addToCache(emoji) + emojis = append(emojis, emoji) + } + } + + return emojis, nil } func (es *LocalCacheEmojiStore) Delete(emoji *model.Emoji, time int64) error { diff --git a/server/channels/store/localcachelayer/emoji_layer_test.go b/server/channels/store/localcachelayer/emoji_layer_test.go index 3c1e7cb0ad..f3b643ff7b 100644 --- a/server/channels/store/localcachelayer/emoji_layer_test.go +++ b/server/channels/store/localcachelayer/emoji_layer_test.go @@ -21,6 +21,7 @@ func TestEmojiStore(t *testing.T) { func TestEmojiStoreCache(t *testing.T) { fakeEmoji := model.Emoji{Id: "123", Name: "name123"} + fakeEmoji2 := model.Emoji{Id: "321", Name: "name321"} ctxEmoji := model.Emoji{Id: "master", Name: "name123"} t.Run("first call by id not cached, second cached and returning same data", func(t *testing.T) { @@ -39,7 +40,7 @@ func TestEmojiStoreCache(t *testing.T) { mockStore.Emoji().(*mocks.EmojiStore).AssertNumberOfCalls(t, "Get", 1) }) - t.Run("first call by name not cached, second cached and returning same data", func(t *testing.T) { + t.Run("GetByName: first call by name not cached, second cached and returning same data", func(t *testing.T) { mockStore := getMockStore() mockCacheProvider := getMockCacheProvider() cachedStore, err := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider) @@ -55,6 +56,43 @@ func TestEmojiStoreCache(t *testing.T) { mockStore.Emoji().(*mocks.EmojiStore).AssertNumberOfCalls(t, "GetByName", 1) }) + t.Run("GetMultipleByName: first call by name not cached, second cached and returning same data", func(t *testing.T) { + mockStore := getMockStore() + mockCacheProvider := getMockCacheProvider() + cachedStore, err := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider) + require.NoError(t, err) + + emojis, err := cachedStore.Emoji().GetMultipleByName(context.Background(), []string{"name123"}) + require.NoError(t, err) + require.Len(t, emojis, 1) + assert.Equal(t, emojis[0], &fakeEmoji) + mockStore.Emoji().(*mocks.EmojiStore).AssertNumberOfCalls(t, "GetMultipleByName", 1) + emojis, err = cachedStore.Emoji().GetMultipleByName(context.Background(), []string{"name123"}) + require.NoError(t, err) + require.Len(t, emojis, 1) + assert.Equal(t, emojis[0], &fakeEmoji) + mockStore.Emoji().(*mocks.EmojiStore).AssertNumberOfCalls(t, "GetMultipleByName", 1) + }) + + t.Run("GetMultipleByName: multiple elements", func(t *testing.T) { + mockStore := getMockStore() + mockCacheProvider := getMockCacheProvider() + cachedStore, err := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider) + require.NoError(t, err) + + emojis, err := cachedStore.Emoji().GetMultipleByName(context.Background(), []string{"name123", "name321"}) + require.NoError(t, err) + require.Len(t, emojis, 2) + assert.Equal(t, emojis[0], &fakeEmoji) + assert.Equal(t, emojis[1], &fakeEmoji2) + mockStore.Emoji().(*mocks.EmojiStore).AssertNumberOfCalls(t, "GetMultipleByName", 1) + emojis, err = cachedStore.Emoji().GetMultipleByName(context.Background(), []string{"name123"}) + require.NoError(t, err) + require.Len(t, emojis, 1) + assert.Equal(t, emojis[0], &fakeEmoji) + mockStore.Emoji().(*mocks.EmojiStore).AssertNumberOfCalls(t, "GetMultipleByName", 1) + }) + t.Run("first call by id not cached, second force not cached", func(t *testing.T) { mockStore := getMockStore() mockCacheProvider := getMockCacheProvider() @@ -107,7 +145,7 @@ func TestEmojiStoreCache(t *testing.T) { mockStore.Emoji().(*mocks.EmojiStore).AssertNumberOfCalls(t, "GetByName", 2) }) - t.Run("first call by id, second call by name cached", func(t *testing.T) { + t.Run("first call by id, second call by name and GetMultipleByName cached", func(t *testing.T) { mockStore := getMockStore() mockCacheProvider := getMockCacheProvider() cachedStore, err := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider) @@ -117,6 +155,8 @@ func TestEmojiStoreCache(t *testing.T) { mockStore.Emoji().(*mocks.EmojiStore).AssertNumberOfCalls(t, "Get", 1) cachedStore.Emoji().GetByName(context.Background(), "name123", true) mockStore.Emoji().(*mocks.EmojiStore).AssertNumberOfCalls(t, "GetByName", 0) + cachedStore.Emoji().GetMultipleByName(context.Background(), []string{"name123"}) + mockStore.Emoji().(*mocks.EmojiStore).AssertNumberOfCalls(t, "GetMultipleByName", 0) }) t.Run("first call by name, second call by id cached", func(t *testing.T) { diff --git a/server/channels/store/localcachelayer/main_test.go b/server/channels/store/localcachelayer/main_test.go index 787dd1d594..4a21a320c4 100644 --- a/server/channels/store/localcachelayer/main_test.go +++ b/server/channels/store/localcachelayer/main_test.go @@ -69,6 +69,7 @@ func getMockStore() *mocks.Store { mockStore.On("Webhook").Return(&mockWebhookStore) fakeEmoji := model.Emoji{Id: "123", Name: "name123"} + fakeEmoji2 := model.Emoji{Id: "321", Name: "name321"} ctxEmoji := model.Emoji{Id: "master", Name: "name123"} mockEmojiStore := mocks.EmojiStore{} mockEmojiStore.On("Get", mock.Anything, "123", true).Return(&fakeEmoji, nil) @@ -77,6 +78,8 @@ func getMockStore() *mocks.Store { mockEmojiStore.On("Get", sqlstore.WithMaster(context.Background()), "master", true).Return(&ctxEmoji, nil) mockEmojiStore.On("GetByName", mock.Anything, "name123", true).Return(&fakeEmoji, nil) mockEmojiStore.On("GetByName", mock.Anything, "name123", false).Return(&fakeEmoji, nil) + mockEmojiStore.On("GetMultipleByName", context.Background(), []string{"name123"}).Return([]*model.Emoji{&fakeEmoji}, nil) + mockEmojiStore.On("GetMultipleByName", context.Background(), []string{"name123", "name321"}).Return([]*model.Emoji{&fakeEmoji, &fakeEmoji2}, nil) mockEmojiStore.On("GetByName", context.Background(), "master", true).Return(&ctxEmoji, nil) mockEmojiStore.On("GetByName", sqlstore.WithMaster(context.Background()), "master", false).Return(&ctxEmoji, nil) mockEmojiStore.On("Delete", &fakeEmoji, int64(0)).Return(nil) diff --git a/server/channels/store/opentracinglayer/opentracinglayer.go b/server/channels/store/opentracinglayer/opentracinglayer.go index 45ab9d90f6..7aa193a317 100644 --- a/server/channels/store/opentracinglayer/opentracinglayer.go +++ b/server/channels/store/opentracinglayer/opentracinglayer.go @@ -3415,7 +3415,7 @@ func (s *OpenTracingLayerEmojiStore) GetList(offset int, limit int, sort string) return result, err } -func (s *OpenTracingLayerEmojiStore) GetMultipleByName(names []string) ([]*model.Emoji, error) { +func (s *OpenTracingLayerEmojiStore) GetMultipleByName(ctx context.Context, names []string) ([]*model.Emoji, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "EmojiStore.GetMultipleByName") s.Root.Store.SetContext(newCtx) @@ -3424,7 +3424,7 @@ func (s *OpenTracingLayerEmojiStore) GetMultipleByName(names []string) ([]*model }() defer span.Finish() - result, err := s.EmojiStore.GetMultipleByName(names) + result, err := s.EmojiStore.GetMultipleByName(ctx, names) if err != nil { span.LogFields(spanlog.Error(err)) ext.Error.Set(span, true) diff --git a/server/channels/store/retrylayer/retrylayer.go b/server/channels/store/retrylayer/retrylayer.go index 740f8ab3f6..7f1f602e91 100644 --- a/server/channels/store/retrylayer/retrylayer.go +++ b/server/channels/store/retrylayer/retrylayer.go @@ -3818,11 +3818,11 @@ func (s *RetryLayerEmojiStore) GetList(offset int, limit int, sort string) ([]*m } -func (s *RetryLayerEmojiStore) GetMultipleByName(names []string) ([]*model.Emoji, error) { +func (s *RetryLayerEmojiStore) GetMultipleByName(ctx context.Context, names []string) ([]*model.Emoji, error) { tries := 0 for { - result, err := s.EmojiStore.GetMultipleByName(names) + result, err := s.EmojiStore.GetMultipleByName(ctx, names) if err == nil { return result, nil } diff --git a/server/channels/store/sqlstore/emoji_store.go b/server/channels/store/sqlstore/emoji_store.go index 9500749770..7429966867 100644 --- a/server/channels/store/sqlstore/emoji_store.go +++ b/server/channels/store/sqlstore/emoji_store.go @@ -52,13 +52,13 @@ func (es SqlEmojiStore) GetByName(ctx context.Context, name string, allowFromCac return es.getBy(ctx, "Name", name) } -func (es SqlEmojiStore) GetMultipleByName(names []string) ([]*model.Emoji, error) { +func (es SqlEmojiStore) GetMultipleByName(ctx context.Context, names []string) ([]*model.Emoji, error) { // Creating (?, ?, ?) len(names) number of times. keys := strings.Join(strings.Fields(strings.Repeat("? ", len(names))), ",") args := makeStringArgs(names) emojis := []*model.Emoji{} - if err := es.GetReplicaX().Select(&emojis, + if err := es.DBXFromContext(ctx).Select(&emojis, `SELECT * FROM diff --git a/server/channels/store/store.go b/server/channels/store/store.go index 27a49f8b1f..3e2798e5e4 100644 --- a/server/channels/store/store.go +++ b/server/channels/store/store.go @@ -669,7 +669,7 @@ type EmojiStore interface { Save(emoji *model.Emoji) (*model.Emoji, error) Get(ctx context.Context, id string, allowFromCache bool) (*model.Emoji, error) GetByName(ctx context.Context, name string, allowFromCache bool) (*model.Emoji, error) - GetMultipleByName(names []string) ([]*model.Emoji, error) + GetMultipleByName(ctx context.Context, names []string) ([]*model.Emoji, error) GetList(offset, limit int, sort string) ([]*model.Emoji, error) Delete(emoji *model.Emoji, timestamp int64) error Search(name string, prefixOnly bool, limit int) ([]*model.Emoji, error) diff --git a/server/channels/store/storetest/emoji_store.go b/server/channels/store/storetest/emoji_store.go index bd39d0fa0f..bfb8ee9c0c 100644 --- a/server/channels/store/storetest/emoji_store.go +++ b/server/channels/store/storetest/emoji_store.go @@ -154,26 +154,26 @@ func testEmojiGetMultipleByName(t *testing.T, ss store.Store) { }() t.Run("one emoji", func(t *testing.T) { - received, err := ss.Emoji().GetMultipleByName([]string{emojis[0].Name}) + received, err := ss.Emoji().GetMultipleByName(context.Background(), []string{emojis[0].Name}) require.NoError(t, err, "could not get emoji") require.Len(t, received, 1, "got incorrect emoji") require.Equal(t, *received[0], emojis[0], "got incorrect emoji") }) t.Run("multiple emojis", func(t *testing.T) { - received, err := ss.Emoji().GetMultipleByName([]string{emojis[0].Name, emojis[1].Name, emojis[2].Name}) + received, err := ss.Emoji().GetMultipleByName(context.Background(), []string{emojis[0].Name, emojis[1].Name, emojis[2].Name}) require.NoError(t, err, "could not get emojis") require.Len(t, received, 3, "got incorrect emojis") }) t.Run("one nonexistent emoji", func(t *testing.T) { - received, err := ss.Emoji().GetMultipleByName([]string{"ab"}) + received, err := ss.Emoji().GetMultipleByName(context.Background(), []string{"ab"}) require.NoError(t, err, "could not get emoji", err) require.Empty(t, received, "got incorrect emoji") }) t.Run("multiple emojis with nonexistent names", func(t *testing.T) { - received, err := ss.Emoji().GetMultipleByName([]string{emojis[0].Name, emojis[1].Name, emojis[2].Name, "abcd", "1234"}) + received, err := ss.Emoji().GetMultipleByName(context.Background(), []string{emojis[0].Name, emojis[1].Name, emojis[2].Name, "abcd", "1234"}) require.NoError(t, err, "could not get emojis") require.Len(t, received, 3, "got incorrect emojis") }) diff --git a/server/channels/store/storetest/mocks/EmojiStore.go b/server/channels/store/storetest/mocks/EmojiStore.go index f9cfe28f03..9c1045a8e3 100644 --- a/server/channels/store/storetest/mocks/EmojiStore.go +++ b/server/channels/store/storetest/mocks/EmojiStore.go @@ -108,25 +108,25 @@ func (_m *EmojiStore) GetList(offset int, limit int, sort string) ([]*model.Emoj return r0, r1 } -// GetMultipleByName provides a mock function with given fields: names -func (_m *EmojiStore) GetMultipleByName(names []string) ([]*model.Emoji, error) { - ret := _m.Called(names) +// GetMultipleByName provides a mock function with given fields: ctx, names +func (_m *EmojiStore) GetMultipleByName(ctx context.Context, names []string) ([]*model.Emoji, error) { + ret := _m.Called(ctx, names) var r0 []*model.Emoji var r1 error - if rf, ok := ret.Get(0).(func([]string) ([]*model.Emoji, error)); ok { - return rf(names) + if rf, ok := ret.Get(0).(func(context.Context, []string) ([]*model.Emoji, error)); ok { + return rf(ctx, names) } - if rf, ok := ret.Get(0).(func([]string) []*model.Emoji); ok { - r0 = rf(names) + if rf, ok := ret.Get(0).(func(context.Context, []string) []*model.Emoji); ok { + r0 = rf(ctx, names) } else { if ret.Get(0) != nil { r0 = ret.Get(0).([]*model.Emoji) } } - if rf, ok := ret.Get(1).(func([]string) error); ok { - r1 = rf(names) + if rf, ok := ret.Get(1).(func(context.Context, []string) error); ok { + r1 = rf(ctx, names) } else { r1 = ret.Error(1) } diff --git a/server/channels/store/timerlayer/timerlayer.go b/server/channels/store/timerlayer/timerlayer.go index 67447d90ad..8fe2245795 100644 --- a/server/channels/store/timerlayer/timerlayer.go +++ b/server/channels/store/timerlayer/timerlayer.go @@ -3128,10 +3128,10 @@ func (s *TimerLayerEmojiStore) GetList(offset int, limit int, sort string) ([]*m return result, err } -func (s *TimerLayerEmojiStore) GetMultipleByName(names []string) ([]*model.Emoji, error) { +func (s *TimerLayerEmojiStore) GetMultipleByName(ctx context.Context, names []string) ([]*model.Emoji, error) { start := time.Now() - result, err := s.EmojiStore.GetMultipleByName(names) + result, err := s.EmojiStore.GetMultipleByName(ctx, names) elapsed := float64(time.Since(start)) / float64(time.Second) if s.Root.Metrics != nil {