From 89b570087d34fa3867724af5ca781117809ed2cf Mon Sep 17 00:00:00 2001 From: dboehm-avalabs Date: Fri, 14 Apr 2023 16:11:36 -0400 Subject: [PATCH 1/4] Update cache.go --- x/merkledb/cache.go | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/x/merkledb/cache.go b/x/merkledb/cache.go index 9aecae740780..115bdf8adf08 100644 --- a/x/merkledb/cache.go +++ b/x/merkledb/cache.go @@ -12,32 +12,25 @@ import ( // A cache that calls [onEviction] on the evicted element. type onEvictCache[K comparable, V any] struct { - lock sync.Mutex - maxSize int - // LRU --> MRU from left to right. - lru linkedhashmap.LinkedHashmap[K, V] + lock sync.RWMutex + maxSize int + fifo linkedhashmap.LinkedHashmap[K, V] onEviction func(V) error } func newOnEvictCache[K comparable, V any](maxSize int, onEviction func(V) error) onEvictCache[K, V] { return onEvictCache[K, V]{ maxSize: maxSize, - lru: linkedhashmap.New[K, V](), + fifo: linkedhashmap.New[K, V](), onEviction: onEviction, } } // Get an element from this cache. func (c *onEvictCache[K, V]) Get(key K) (V, bool) { - c.lock.Lock() - defer c.lock.Unlock() - - val, ok := c.lru.Get(key) - if ok { - // This key was touched; move it to the MRU position. - c.lru.Put(key, val) - } - return val, ok + c.lock.RLock() + defer c.lock.RUnlock() + return c.fifo.Get(key) } // Put an element into this cache. If this causes an element @@ -47,13 +40,13 @@ func (c *onEvictCache[K, V]) Put(key K, value V) error { c.lock.Lock() defer c.lock.Unlock() - c.lru.Put(key, value) // Mark as MRU + c.fifo.Put(key, value) // Mark as MRU - if c.lru.Len() > c.maxSize { + if c.fifo.Len() > c.maxSize { // Note that [c.cache] has already evicted the oldest // element because its max size is [c.maxSize]. - oldestKey, oldsetVal, _ := c.lru.Oldest() - c.lru.Delete(oldestKey) + oldestKey, oldsetVal, _ := c.fifo.Oldest() + c.fifo.Delete(oldestKey) return c.onEviction(oldsetVal) } return nil @@ -66,12 +59,12 @@ func (c *onEvictCache[K, V]) Put(key K, value V) error { func (c *onEvictCache[K, V]) Flush() error { c.lock.Lock() defer func() { - c.lru = linkedhashmap.New[K, V]() + c.fifo = linkedhashmap.New[K, V]() c.lock.Unlock() }() var errs wrappers.Errs - iter := c.lru.NewIterator() + iter := c.fifo.NewIterator() for iter.Next() { val := iter.Value() errs.Add(c.onEviction(val)) From c3bafc2622bc8898668b80c68585149db5b87b3a Mon Sep 17 00:00:00 2001 From: dboehm-avalabs Date: Fri, 14 Apr 2023 16:16:54 -0400 Subject: [PATCH 2/4] Update cache_test.go --- x/merkledb/cache_test.go | 62 ++++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/x/merkledb/cache_test.go b/x/merkledb/cache_test.go index a841aef7163f..3179f0126bb9 100644 --- a/x/merkledb/cache_test.go +++ b/x/merkledb/cache_test.go @@ -24,8 +24,8 @@ func TestNewOnEvictCache(t *testing.T) { cache := newOnEvictCache[int](maxSize, onEviction) require.Equal(maxSize, cache.maxSize) - require.NotNil(cache.lru) - require.Equal(0, cache.lru.Len()) + require.NotNil(cache.fifo) + require.Equal(0, cache.fifo.Len()) // Can't test function equality directly so do this // to make sure it was assigned correctly err := cache.onEviction(0) @@ -35,7 +35,7 @@ func TestNewOnEvictCache(t *testing.T) { // Test the functionality of the cache when the onEviction function // never returns an error. -// Note this test assumes the internal cache is an LRU cache. +// Note this test assumes the internal cache is a FIFO cache func TestOnEvictCacheNoOnEvictionError(t *testing.T) { require := require.New(t) @@ -55,7 +55,7 @@ func TestOnEvictCacheNoOnEvictionError(t *testing.T) { // Put key err := cache.Put(0, 0) require.NoError(err) - require.Equal(1, cache.lru.Len()) + require.Equal(1, cache.fifo.Len()) // Get key val, ok := cache.Get(0) @@ -70,21 +70,21 @@ func TestOnEvictCacheNoOnEvictionError(t *testing.T) { for i := 1; i < maxSize; i++ { err := cache.Put(i, i) require.NoError(err) - require.Equal(i+1, cache.lru.Len()) + require.Equal(i+1, cache.fifo.Len()) } require.Len(evicted, 0) - // Cache has [0,1,2] from LRU --> MRU + // Cache has [0,1,2] - // Put another key. This should evict the LRU key (0). + // Put another key. This should evict the oldest inserted key (0). err = cache.Put(maxSize, maxSize) require.NoError(err) - require.Equal(maxSize, cache.lru.Len()) + require.Equal(maxSize, cache.fifo.Len()) require.Len(evicted, 1) require.Equal(0, evicted[0]) - // Cache has [1,2,3] from LRU --> MRU - iter := cache.lru.NewIterator() + // Cache has [1,2,3] + iter := cache.fifo.NewIterator() require.True(iter.Next()) require.Equal(1, iter.Key()) require.Equal(1, iter.Value()) @@ -107,51 +107,51 @@ func TestOnEvictCacheNoOnEvictionError(t *testing.T) { require.Equal(i, val) } - // Cache has [3,2,1] from LRU --> MRU - iter = cache.lru.NewIterator() + // Cache has [1,2,3] + iter = cache.fifo.NewIterator() require.True(iter.Next()) - require.Equal(3, iter.Key()) - require.Equal(3, iter.Value()) + require.Equal(1, iter.Key()) + require.Equal(1, iter.Value()) require.True(iter.Next()) require.Equal(2, iter.Key()) require.Equal(2, iter.Value()) require.True(iter.Next()) - require.Equal(1, iter.Key()) - require.Equal(1, iter.Value()) + require.Equal(3, iter.Key()) + require.Equal(3, iter.Value()) require.False(iter.Next()) - // Put another key to evict the LRU key (3). + // Put another key to evict the oldest inserted key (1). err = cache.Put(maxSize+1, maxSize+1) require.NoError(err) - require.Equal(maxSize, cache.lru.Len()) + require.Equal(maxSize, cache.fifo.Len()) require.Len(evicted, 2) - require.Equal(3, evicted[1]) + require.Equal(1, evicted[1]) - // Cache has [2,1,4] from LRU --> MRU - iter = cache.lru.NewIterator() + // Cache has [2,3,4] + iter = cache.fifo.NewIterator() require.True(iter.Next()) require.Equal(2, iter.Key()) require.Equal(2, iter.Value()) require.True(iter.Next()) - require.Equal(1, iter.Key()) - require.Equal(1, iter.Value()) + require.Equal(3, iter.Key()) + require.Equal(3, iter.Value()) require.True(iter.Next()) require.Equal(4, iter.Key()) require.Equal(4, iter.Value()) require.False(iter.Next()) - // 3 should no longer be in the cache - _, ok = cache.Get(3) + // 1 should no longer be in the cache + _, ok = cache.Get(1) require.False(ok) err = cache.Flush() require.NoError(err) // Cache should be empty - require.Equal(0, cache.lru.Len()) + require.Equal(0, cache.fifo.Len()) require.Len(evicted, 5) - require.Equal(evicted, []int{0, 3, 2, 1, 4}) - require.Equal(0, cache.lru.Len()) + require.Equal([]int{0, 1, 2, 3, 4}, evicted) + require.Equal(0, cache.fifo.Len()) require.Equal(maxSize, cache.maxSize) // Should be unchanged } @@ -179,7 +179,7 @@ func TestOnEvictCacheOnEvictionError(t *testing.T) { for i := 0; i < maxSize; i++ { err := cache.Put(i, i) require.NoError(err) - require.Equal(i+1, cache.lru.Len()) + require.Equal(i+1, cache.fifo.Len()) } // Put another key. This should evict the LRU key (0) @@ -189,7 +189,7 @@ func TestOnEvictCacheOnEvictionError(t *testing.T) { // Cache should still have correct state [1,2] require.Equal(evicted, []int{0}) - require.Equal(maxSize, cache.lru.Len()) + require.Equal(maxSize, cache.fifo.Len()) _, ok := cache.Get(0) require.False(ok) _, ok = cache.Get(1) @@ -202,7 +202,7 @@ func TestOnEvictCacheOnEvictionError(t *testing.T) { require.ErrorIs(err, errTest) // Should still be empty. - require.Equal(0, cache.lru.Len()) + require.Equal(0, cache.fifo.Len()) require.Equal(evicted, []int{0, 1, 2}) _, ok = cache.Get(0) require.False(ok) From 389ac2aa62bec0affb85a130274184521f42845c Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Mon, 17 Apr 2023 11:01:22 -0400 Subject: [PATCH 3/4] add newline; remove old comment --- x/merkledb/cache.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/x/merkledb/cache.go b/x/merkledb/cache.go index 115bdf8adf08..593278ef722e 100644 --- a/x/merkledb/cache.go +++ b/x/merkledb/cache.go @@ -30,6 +30,7 @@ func newOnEvictCache[K comparable, V any](maxSize int, onEviction func(V) error) func (c *onEvictCache[K, V]) Get(key K) (V, bool) { c.lock.RLock() defer c.lock.RUnlock() + return c.fifo.Get(key) } @@ -43,8 +44,6 @@ func (c *onEvictCache[K, V]) Put(key K, value V) error { c.fifo.Put(key, value) // Mark as MRU if c.fifo.Len() > c.maxSize { - // Note that [c.cache] has already evicted the oldest - // element because its max size is [c.maxSize]. oldestKey, oldsetVal, _ := c.fifo.Oldest() c.fifo.Delete(oldestKey) return c.onEviction(oldsetVal) From a0662c0045f20ac86f3dd0775a3d4f5afe145060 Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Mon, 17 Apr 2023 11:05:12 -0400 Subject: [PATCH 4/4] update test comments --- x/merkledb/cache_test.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/x/merkledb/cache_test.go b/x/merkledb/cache_test.go index 3179f0126bb9..fb35c27afcff 100644 --- a/x/merkledb/cache_test.go +++ b/x/merkledb/cache_test.go @@ -157,7 +157,7 @@ func TestOnEvictCacheNoOnEvictionError(t *testing.T) { // Test the functionality of the cache when the onEviction function // returns an error. -// Note this test assumes the internal cache is an LRU cache. +// Note this test assumes the cache is FIFO. func TestOnEvictCacheOnEvictionError(t *testing.T) { var ( require = require.New(t) @@ -182,12 +182,14 @@ func TestOnEvictCacheOnEvictionError(t *testing.T) { require.Equal(i+1, cache.fifo.Len()) } - // Put another key. This should evict the LRU key (0) + // Cache has [0,1] + + // Put another key. This should evict the first key (0) // and return an error since 0 is even. err := cache.Put(maxSize, maxSize) require.ErrorIs(err, errTest) - // Cache should still have correct state [1,2] + // Cache has [1,2] require.Equal(evicted, []int{0}) require.Equal(maxSize, cache.fifo.Len()) _, ok := cache.Get(0)