daemon: improve revcache performance (#4607)

When dealing with a lot of paths the current implementation is not
optimal. Specifically filterRevoked showed up quite prominently in a
pprof. See image below:


![image](https://github.com/user-attachments/assets/23b337e7-dece-4191-bcf5-75761de93a83)


- Use a typed cache, to prevent string key conversion.
- Only support single key lookups to prevent map allocation on lookup.

Overall this even simplifies the code.

go.mod

@@ -54,6 +54,7 @@ require (
 	google.golang.org/protobuf v1.34.1
 	gopkg.in/yaml.v2 v2.4.0
 	modernc.org/sqlite v1.29.9
+	zgo.at/zcache/v2 v2.1.0
 )
 
 require (

go.sum

@@ -497,3 +497,5 @@ modernc.org/strutil v1.2.0/go.mod h1:/mdcBmfOibveCTBxUl5B5l6W+TTH1FXPLHZE6bTosX0
 modernc.org/token v1.1.0 h1:Xl7Ap9dKaEs5kLoOQeQmPWevfnk/DM5qcLcYlA8ys6Y=
 modernc.org/token v1.1.0/go.mod h1:UGzOrNV1mAFSEB63lOFHIpNRUVMvYTc6yu1SMY/XTDM=
 rsc.io/pdf v0.1.1/go.mod h1:n8OzWcQ6Sp37PL01nO98y4iUCRdTGarVfzxY20ICaU4=
+zgo.at/zcache/v2 v2.1.0 h1:USo+ubK+R4vtjw4viGzTe/zjXyPw6R7SK/RL3epBBxs=
+zgo.at/zcache/v2 v2.1.0/go.mod h1:gyCeoLVo01QjDZynjime8xUGHHMbsLiPyUTBpDGd4Gk=

go_deps.bzl

@@ -7,6 +7,12 @@
 load("@bazel_gazelle//:deps.bzl", "go_repository")
 
 def go_deps():
+    go_repository(
+        name = "at_zgo_zcache_v2",
+        importpath = "zgo.at/zcache/v2",
+        sum = "h1:USo+ubK+R4vtjw4viGzTe/zjXyPw6R7SK/RL3epBBxs=",
+        version = "v2.1.0",
+    )
     go_repository(
         name = "co_honnef_go_tools",
         importpath = "honnef.co/go/tools",

licenses/data/at_zgo_zcache_v2/LICENSE

@@ -0,0 +1,19 @@
+Copyright (c) 2012-2019 Patrick Mylund Nielsen and the go-cache contributors
+
+Permission is hereby granted, free of charge, to any person obtaining a copy
+of this software and associated documentation files (the "Software"), to deal
+in the Software without restriction, including without limitation the rights
+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+copies of the Software, and to permit persons to whom the Software is
+furnished to do so, subject to the following conditions:
+
+The above copyright notice and this permission notice shall be included in
+all copies or substantial portions of the Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+THE SOFTWARE.

private/revcache/memrevcache/BUILD.bazel

@@ -8,7 +8,7 @@ go_library(
     deps = [
         "//pkg/private/ctrl/path_mgmt:go_default_library",
         "//private/revcache:go_default_library",
-        "@com_github_patrickmn_go_cache//:go_default_library",
+        "@at_zgo_zcache_v2//:go_default_library",
     ],
 )
 

private/revcache/memrevcache/memrevcache.go

@@ -19,7 +19,7 @@ import (
 	"sync"
 	"time"
 
-	cache "github.com/patrickmn/go-cache"
+	cache "zgo.at/zcache/v2"
 
 	"github.com/scionproto/scion/pkg/private/ctrl/path_mgmt"
 	"github.com/scionproto/scion/private/revcache"
@@ -29,7 +29,7 @@ var _ revcache.RevCache = (*memRevCache)(nil)
 
 type memRevCache struct {
 	// Do not embed or use type directly to reduce the cache's API surface
-	c    *cache.Cache
+	c    *cache.Cache[revcache.Key, *path_mgmt.RevInfo]
 	lock sync.RWMutex
 }
 
@@ -38,20 +38,17 @@ func New() *memRevCache {
 	return &memRevCache{
 		// We insert all the items with expiration so no need to have a default expiration.
 		// The cleaning should happen manually using the DeleteExpired method.
-		c: cache.New(cache.NoExpiration, 0),
+		c: cache.New[revcache.Key, *path_mgmt.RevInfo](cache.NoExpiration, 0),
 	}
 }
 
-func (c *memRevCache) Get(_ context.Context, keys revcache.KeySet) (revcache.Revocations, error) {
+func (c *memRevCache) Get(_ context.Context, key revcache.Key) (*path_mgmt.RevInfo, error) {
 	c.lock.RLock()
 	defer c.lock.RUnlock()
-	revs := make(revcache.Revocations, len(keys))
-	for k := range keys {
-		if revInfo, ok := c.get(k.String()); ok {
-			revs[k] = revInfo
-		}
+	if revInfo, ok := c.c.Get(key); ok {
+		return revInfo, nil
 	}
-	return revs, nil
+	return nil, nil
 }
 
 func (c *memRevCache) GetAll(_ context.Context) (revcache.ResultChan, error) {
@@ -61,38 +58,27 @@ func (c *memRevCache) GetAll(_ context.Context) (revcache.ResultChan, error) {
 	items := c.c.Items()
 	resCh := make(chan revcache.RevOrErr, len(items))
 	for _, item := range items {
-		if rev, ok := item.Object.(*path_mgmt.RevInfo); ok {
-			resCh <- revcache.RevOrErr{Rev: rev}
-		}
+		resCh <- revcache.RevOrErr{Rev: item.Object}
 	}
 	close(resCh)
 	return resCh, nil
 }
 
-func (c *memRevCache) get(key string) (*path_mgmt.RevInfo, bool) {
-	obj, ok := c.c.Get(key)
-	if !ok {
-		return nil, false
-	}
-	return obj.(*path_mgmt.RevInfo), true
-}
-
 func (c *memRevCache) Insert(_ context.Context, rev *path_mgmt.RevInfo) (bool, error) {
 	c.lock.Lock()
 	defer c.lock.Unlock()
 	ttl := time.Until(rev.Expiration())
 	if ttl <= 0 {
 		return false, nil
 	}
-	k := revcache.NewKey(rev.IA(), rev.IfID)
-	key := k.String()
-	val, ok := c.get(key)
+	key := revcache.NewKey(rev.IA(), rev.IfID)
+	val, ok := c.c.Get(key)
 	if !ok {
-		c.c.Set(key, rev, ttl)
+		c.c.SetWithExpire(key, rev, ttl)
 		return true, nil
 	}
 	if rev.Timestamp().After(val.Timestamp()) {
-		c.c.Set(key, rev, ttl)
+		c.c.SetWithExpire(key, rev, ttl)
 		return true, nil
 	}
 	return false, nil
@@ -102,7 +88,7 @@ func (c *memRevCache) DeleteExpired(_ context.Context) (int64, error) {
 	c.lock.Lock()
 	defer c.lock.Unlock()
 	var cnt int64
-	c.c.OnEvicted(func(string, interface{}) {
+	c.c.OnEvicted(func(revcache.Key, *path_mgmt.RevInfo) {
 		cnt++
 	})
 	c.c.DeleteExpired()

private/revcache/memrevcache/memrevcache_test.go

@@ -37,9 +37,8 @@ func (c *testRevCache) InsertExpired(t *testing.T, _ context.Context,
 	if ttl >= 0 {
 		panic("Should only be used for expired elements")
 	}
-	k := revcache.NewKey(rev.IA(), rev.IfID)
-	key := k.String()
-	c.c.Set(key, rev, time.Microsecond)
+	key := revcache.NewKey(rev.IA(), rev.IfID)
+	c.c.SetWithExpire(key, rev, time.Microsecond)
 	// Unfortunately inserting with negative TTL makes entries available forever,
 	// so we use 1 micro second and sleep afterwards
 	// to simulate the insertion of an expired entry.

private/revcache/revcache.go

@@ -43,14 +43,6 @@ func (k Key) String() string {
 	return fmt.Sprintf("%s#%s", k.IA, k.IfID)
 }
 
-// KeySet is a set of keys.
-type KeySet map[Key]struct{}
-
-// SingleKey is a convenience function to return a KeySet with a single key.
-func SingleKey(ia addr.IA, ifID common.IfIDType) KeySet {
-	return KeySet{Key{IA: ia, IfID: ifID}: {}}
-}
-
 // RevOrErr is either a revocation or an error.
 type RevOrErr struct {
 	Rev *path_mgmt.RevInfo
@@ -64,7 +56,7 @@ type ResultChan <-chan RevOrErr
 type RevCache interface {
 	// Get items with the given keys from the cache. Returns all present requested items that are
 	// not expired or an error if the query failed.
-	Get(ctx context.Context, keys KeySet) (Revocations, error)
+	Get(ctx context.Context, key Key) (*path_mgmt.RevInfo, error)
 	// GetAll returns a channel that will provide all items in the revocation cache. If the cache
 	// can't prepare the result channel a nil channel and the error are returned. If the querying
 	// succeeded the channel will contain the revocations in the cache, or an error if the stored
@@ -83,6 +75,3 @@ type RevCache interface {
 	db.LimitSetter
 	io.Closer
 }
-
-// Revocations is the map of revocations.
-type Revocations map[Key]*path_mgmt.RevInfo

private/revcache/revcachetest/revcachetest.go

@@ -85,16 +85,15 @@ func testInsertGet(t *testing.T, revCache TestableRevCache) {
 	assert.True(t, inserted, "Insert should return true for a new entry")
 	assert.NoError(t, err, "Insert a new entry should not err")
 	key1 := revcache.NewKey(ia110, ifID15)
-	revs, err := revCache.Get(ctx, revcache.KeySet{key1: {}})
+	aRev, err := revCache.Get(ctx, key1)
 	assert.NoError(t, err, "Get should not err for existing entry")
-	assert.NotEmpty(t, revs, "Get should return existing entry")
-	assert.Equal(t, rev, revs[key1], "Get should return previously inserted value")
+	assert.Equal(t, rev, aRev, "Get should return previously inserted value")
 	inserted, err = revCache.Insert(ctx, rev)
 	assert.False(t, inserted, "Insert should return false for already existing entry")
 	assert.NoError(t, err, "Insert should not err")
-	revs, err = revCache.Get(ctx, revcache.SingleKey(ia110, ifID19))
+	aRev, err = revCache.Get(ctx, revcache.NewKey(ia110, ifID19))
 	assert.NoError(t, err, "Get should not err")
-	assert.Empty(t, revs, "Get should return empty result for not present value")
+	assert.Nil(t, aRev, "Get should return empty result for not present value")
 }
 
 func testGetMultikey(t *testing.T, revCache TestableRevCache) {
@@ -105,12 +104,7 @@ func testGetMultikey(t *testing.T, revCache TestableRevCache) {
 	ctx, cancelF := context.WithTimeout(context.Background(), TimeOut)
 	defer cancelF()
 
-	// First test the empty cache
-	revs, err := revCache.Get(ctx, revcache.KeySet{})
-	assert.NoError(t, err, "Get should not err")
-	assert.Empty(t, revs, "Should return no revs")
-
-	_, err = revCache.Insert(ctx, rev1)
+	_, err := revCache.Insert(ctx, rev1)
 	require.NoError(t, err)
 	_, err = revCache.Insert(ctx, rev2)
 	require.NoError(t, err)
@@ -120,22 +114,19 @@ func testGetMultikey(t *testing.T, revCache TestableRevCache) {
 	require.NoError(t, err)
 
 	key1 := revcache.NewKey(ia110, ifID15)
-	revs, err = revCache.Get(ctx, revcache.KeySet{key1: {}})
+	aRev1, err := revCache.Get(ctx, key1)
 	assert.NoError(t, err, "Get should not err")
-	assert.Equal(t, len(revs), 1, "Should contain one rev")
-	assert.Equal(t, revcache.Revocations{key1: rev1}, revs,
-		"Get should return revs for the given keys")
+	assert.Equal(t, rev1, aRev1, "Get should return rev for the given keys")
 
 	key2 := revcache.NewKey(ia110, ifID19)
 	key3 := revcache.NewKey(ia120, ifID15)
 	key4 := revcache.NewKey(ia120, ifID19) // not the key of sr4
-	searchKeys := revcache.KeySet{key1: {}, key2: {}, key3: {}, key4: {}}
-	revs, err = revCache.Get(ctx, searchKeys)
-	assert.NoError(t, err, "Get should not err")
-	expectedResult := revcache.Revocations{
-		key1: rev1, key2: rev2, key3: rev3,
+	searchKeys := map[revcache.Key]*path_mgmt.RevInfo{key1: rev1, key2: rev2, key3: rev3, key4: nil}
+	for key, eRev := range searchKeys {
+		aRev, err := revCache.Get(ctx, key)
+		assert.NoError(t, err, "Get should not err")
+		assert.Equal(t, eRev, aRev, "Get should return the requested revocation for key %s", key)
 	}
-	assert.Equal(t, expectedResult, revs, "Get should return the requested revocations")
 }
 
 func testGetAll(t *testing.T, revCache TestableRevCache) {
@@ -235,10 +226,9 @@ func testInsertNewer(t *testing.T, revCache TestableRevCache) {
 	assert.True(t, inserted, "Insert should return true for a new entry")
 	assert.NoError(t, err, "Insert a new entry should not err")
 	key1 := revcache.NewKey(ia110, ifID15)
-	revs, err := revCache.Get(ctx, revcache.KeySet{key1: {}})
+	aRev, err := revCache.Get(ctx, key1)
 	assert.NoError(t, err, "Get should not err for existing entry")
-	assert.NotEmpty(t, revs, "Get should return non empty map for inserted value")
-	assert.Equal(t, revNew, revs[key1], "Get should return previously inserted value")
+	assert.Equal(t, revNew, aRev, "Get should return previously inserted value")
 }
 
 func testGetExpired(t *testing.T, revCache TestableRevCache) {
@@ -252,8 +242,8 @@ func testGetExpired(t *testing.T, revCache TestableRevCache) {
 		RawTTL:       1,
 	}
 	revCache.InsertExpired(t, ctx, revNew)
-	revs, err := revCache.Get(ctx, revcache.SingleKey(ia110, ifID15))
-	assert.Empty(t, revs, "Expired entry should not be returned")
+	aRev, err := revCache.Get(ctx, revcache.NewKey(ia110, ifID15))
+	assert.Nil(t, aRev, "Expired entry should not be returned")
 	assert.NoError(t, err, "Should not error for expired entry")
 }
 
@@ -272,13 +262,12 @@ func testGetMuliKeysExpired(t *testing.T, revCache TestableRevCache) {
 	_, err := revCache.Insert(ctx, rev110_19)
 	assert.NoError(t, err)
 	validKey := revcache.NewKey(ia110, ifID19)
-	srCache, err := revCache.Get(ctx, revcache.KeySet{
-		revcache.NewKey(ia110, ifID15): {},
-		validKey:                       {},
-	})
+	aRev, err := revCache.Get(ctx, validKey)
+	assert.NoError(t, err, "Should not error for valid entry")
+	assert.Equal(t, rev110_19, aRev, "Valid entry should be returned")
+	aRev, err = revCache.Get(ctx, revcache.NewKey(ia110, ifID15))
+	assert.Nil(t, aRev, "Expired entry should not be returned")
 	assert.NoError(t, err, "Should not error for expired entry")
-	assert.Equal(t, revcache.Revocations{validKey: rev110_19}, srCache,
-		"Expired entry should not be returned")
 }
 
 func testDeleteExpired(t *testing.T, revCache TestableRevCache) {

private/revcache/util.go

@@ -17,7 +17,6 @@ package revcache
 import (
 	"context"
 
-	"github.com/scionproto/scion/pkg/addr"
 	"github.com/scionproto/scion/pkg/private/common"
 	seg "github.com/scionproto/scion/pkg/segment"
 	"github.com/scionproto/scion/private/storage/cleaner"
@@ -35,33 +34,20 @@ func NewCleaner(rc RevCache, s string) *cleaner.Cleaner {
 func NoRevokedHopIntf(ctx context.Context, revCache RevCache,
 	s *seg.PathSegment) (bool, error) {
 
-	revKeys := make(KeySet)
-	addRevKeys([]*seg.PathSegment{s}, revKeys, true)
-	revs, err := revCache.Get(ctx, revKeys)
-	return len(revs) == 0, err
-}
-
-// addRevKeys adds all revocations keys for the given segments to the keys set.
-// If hopOnly is set, only the first hop entry is considered.
-func addRevKeys(segs []*seg.PathSegment, keys KeySet, hopOnly bool) {
-	addIntfs := func(ia addr.IA, ingress, egress uint16) {
-		if ingress != 0 {
-			keys[Key{IA: ia, IfID: common.IfIDType(ingress)}] = struct{}{}
-		}
-		if egress != 0 {
-			keys[Key{IA: ia, IfID: common.IfIDType(egress)}] = struct{}{}
-		}
-	}
-	for _, s := range segs {
-		for _, asEntry := range s.ASEntries {
-			hop := asEntry.HopEntry.HopField
-			addIntfs(asEntry.Local, hop.ConsIngress, hop.ConsEgress)
-			if hopOnly {
-				continue
+	for _, asEntry := range s.ASEntries {
+		hop := asEntry.HopEntry.HopField
+		for _, key := range [2]Key{
+			{IA: asEntry.Local, IfID: common.IfIDType(hop.ConsIngress)},
+			{IA: asEntry.Local, IfID: common.IfIDType(hop.ConsEgress)},
+		} {
+			rev, err := revCache.Get(ctx, key)
+			if err != nil || rev != nil {
+				return false, err
 			}
-			for _, peer := range asEntry.PeerEntries {
-				addIntfs(asEntry.Local, peer.HopField.ConsIngress, peer.HopField.ConsEgress)
+			if rev != nil {
+				return false, nil
 			}
 		}
 	}
+	return true, nil
 }