MM-27456: Use reflect-free serialization for hot structs (#15171)

Automatic Merge

services/cache/lru.go

@@ -8,6 +8,8 @@ import (
 	"sync"
 	"time"
 
+	"github.com/mattermost/mattermost-server/v5/model"
+	"github.com/tinylib/msgp/msgp"
 	"github.com/vmihailenco/msgpack/v5"
 )
 
@@ -141,9 +143,21 @@ func (l *LRU) set(key string, value interface{}, ttl time.Duration) error {
 		expires = time.Now().Add(ttl)
 	}
 
-	buf, err := msgpack.Marshal(value)
-	if err != nil {
-		return err
+	var buf []byte
+	var err error
+
+	// We use a fast path for hot structs.
+	if msgpVal, ok := value.(msgp.Marshaler); ok {
+		buf, err = msgpVal.MarshalMsg(nil)
+		if err != nil {
+			return err
+		}
+	} else {
+		// Slow path for other structs.
+		buf, err = msgpack.Marshal(value)
+		if err != nil {
+			return err
+		}
 	}
 
 	// Check for existing item, ignoring expiry since we'd update anyway.
@@ -182,6 +196,34 @@ func (l *LRU) get(key string, value interface{}) error {
 
 		l.evictList.MoveToFront(ent)
 
+		// We use a fast path for hot structs.
+		if msgpVal, ok := value.(msgp.Unmarshaler); ok {
+			_, err := msgpVal.UnmarshalMsg(e.value)
+			return err
+		}
+
+		// This is ugly and makes the cache package aware of the model package.
+		// But this is due to 2 things.
+		// 1. The msgp package works on methods on structs rather than functions.
+		// 2. Our cache interface passes pointers to empty pointers, and not pointers
+		// to values. This is mainly how all our model structs are passed around.
+		// It might be technically possible to use values _just_ for hot structs
+		// like these and then return a pointer while returning from the cache function,
+		// but it will make the codebase inconsistent, and has some edge-cases to take care of.
+		switch v := value.(type) {
+		case **model.User:
+			var u model.User
+			_, err := u.UnmarshalMsg(e.value)
+			*v = &u
+			return err
+		case **model.Session:
+			var s model.Session
+			_, err := s.UnmarshalMsg(e.value)
+			*v = &s
+			return err
+		}
+
+		// Slow path for other structs.
 		return msgpack.Unmarshal(e.value, value)
 	}
 	return ErrKeyNotFound

services/cache/lru_test.go

@@ -205,6 +205,80 @@ func TestLRUMarshalUnMarshal(t *testing.T) {
 	err = l.Get("post", &p)
 	require.Nil(t, err)
 	require.Equal(t, post.Clone(), p.Clone())
+
+	session := &model.Session{
+		Id:             "ty7ia14yuty5bmpt8wmz6da1fw",
+		Token:          "79c3iq6nzpycmkkawudanqhg5c",
+		CreateAt:       1595445296960,
+		ExpiresAt:      1598296496960,
+		LastActivityAt: 1595445296960,
+		UserId:         "rpgh1q5ra38y9xjn9z8fjctezr",
+		Roles:          "system_admin system_user",
+		IsOAuth:        false,
+		ExpiredNotify:  false,
+		Props: map[string]string{
+			"csrf":     "33zb7h7rk3rfffztojn5pxbkxe",
+			"isMobile": "false",
+			"isSaml":   "false",
+			"is_guest": "false",
+			"os":       "",
+			"platform": "Windows",
+		},
+	}
+
+	err = l.Set("session", session)
+	require.Nil(t, err)
+
+	var s *model.Session
+	err = l.Get("session", &s)
+	require.Nil(t, err)
+	require.Equal(t, session, s)
+
+	user := &model.User{
+		Id:             "id",
+		CreateAt:       11111,
+		UpdateAt:       11111,
+		DeleteAt:       11111,
+		Username:       "username",
+		Password:       "password",
+		AuthService:    "AuthService",
+		AuthData:       nil,
+		Email:          "Email",
+		EmailVerified:  true,
+		Nickname:       "Nickname",
+		FirstName:      "FirstName",
+		LastName:       "LastName",
+		Position:       "Position",
+		Roles:          "Roles",
+		AllowMarketing: true,
+		Props: map[string]string{
+			"key0": "value0",
+		},
+		NotifyProps: map[string]string{
+			"key0": "value0",
+		},
+		LastPasswordUpdate:     111111,
+		LastPictureUpdate:      111111,
+		FailedAttempts:         111111,
+		Locale:                 "Locale",
+		MfaActive:              true,
+		MfaSecret:              "MfaSecret",
+		LastActivityAt:         111111,
+		IsBot:                  true,
+		TermsOfServiceId:       "TermsOfServiceId",
+		TermsOfServiceCreateAt: 111111,
+	}
+
+	err = l.Set("user", user)
+	require.Nil(t, err)
+
+	var u *model.User
+	err = l.Get("user", &u)
+	require.Nil(t, err)
+	// msgp returns an empty map instead of a nil map.
+	// This does not make an actual difference in terms of functionality.
+	u.Timezone = nil
+	require.Equal(t, user, u)
 }
 
 func BenchmarkLRU(b *testing.B) {
@@ -458,7 +532,43 @@ func BenchmarkLRU(b *testing.B) {
 			err := l2.Set("test", status)
 			require.Nil(b, err)
 
-			var val model.Status
+			var val *model.Status
+			err = l2.Get("test", &val)
+			require.Nil(b, err)
+		}
+	})
+
+	session := model.Session{
+		Id:             "ty7ia14yuty5bmpt8wmz6da1fw",
+		Token:          "79c3iq6nzpycmkkawudanqhg5c",
+		CreateAt:       1595445296960,
+		ExpiresAt:      1598296496960,
+		LastActivityAt: 1595445296960,
+		UserId:         "rpgh1q5ra38y9xjn9z8fjctezr",
+		Roles:          "system_admin system_user",
+		IsOAuth:        false,
+		ExpiredNotify:  false,
+		Props: map[string]string{
+			"csrf":     "33zb7h7rk3rfffztojn5pxbkxe",
+			"isMobile": "false",
+			"isSaml":   "false",
+			"is_guest": "false",
+			"os":       "",
+			"platform": "Windows",
+		},
+	}
+
+	b.Run("Session=new", func(b *testing.B) {
+		for i := 0; i < b.N; i++ {
+			l2 := NewLRU(&LRUOptions{
+				Size:                   1,
+				DefaultExpiry:          0,
+				InvalidateClusterEvent: "",
+			})
+			err := l2.Set("test", &session)
+			require.Nil(b, err)
+
+			var val *model.Session
 			err = l2.Get("test", &val)
 			require.Nil(b, err)
 		}