Repository: buchgr/bazel-remote
Pull request #372 (2020-10-26 16:36:37)

lru cleanup


Patch:
diff --git a/cache/disk/disk.go b/cache/disk/disk.go
index bd5e459..a0cb13b 100644
--- a/cache/disk/disk.go
+++ b/cache/disk/disk.go
@@ -41,15 +41,10 @@ var (
 var tfc = tempfile.NewCreator()
 
 // lruItem is the type of the values stored in SizedLRU to keep track of items.
-// It implements the sizedItem interface.
 type lruItem struct {
 	size int64
 }
 
-func (i *lruItem) Size() int64 {
-	return i.size
-}
-
 // Cache is a filesystem-based LRU cache, with an optional backend proxy.
 type Cache struct {
 	dir   string
@@ -94,7 +89,7 @@ func New(dir string, maxSizeBytes int64, proxy cache.Proxy) *Cache {
 	// The eviction callback deletes the file from disk.
 	// This function is only called while the lock is held
 	// by the current goroutine.
-	onEvict := func(key Key, value sizedItem) {
+	onEvict := func(key Key, value lruItem) {
 		f := filepath.Join(dir, key.(string))
 		err := os.Remove(f)
 		if err != nil {
@@ -214,7 +209,7 @@ func (c *Cache) loadExistingFiles() error {
 	log.Println("Building LRU index.")
 	for _, f := range files {
 		relPath := f.name[len(c.dir)+1:]
-		ok := c.lru.Add(relPath, &lruItem{size: f.info.Size()})
+		ok := c.lru.Add(relPath, lruItem{size: f.info.Size()})
 		if !ok {
 			err = os.Remove(filepath.Join(c.dir, relPath))
 			if err != nil {
@@ -388,7 +383,7 @@ func (c *Cache) commit(key string, tempfile string, finalPath string, reservedSi
 	}
 	unreserve = false
 
-	if !c.lru.Add(key, &lruItem{size: foundSize}) {
+	if !c.lru.Add(key, lruItem{size: foundSize}) {
 		err = fmt.Errorf("INTERNAL ERROR: failed to add: %s, size %d", key, foundSize)
 		log.Println(err.Error())
 		return unreserve, removeTempfile, err
@@ -415,12 +410,11 @@ func (c *Cache) availableOrTryProxy(key string, size int64, blobPath string) (rc
 	locked := true
 	c.mu.Lock()
 
-	val, available := c.lru.Get(key)
+	item, available := c.lru.Get(key)
 	if available {
 		c.mu.Unlock() // We expect a cache hit below.
 		locked = false
 
-		item := val.(*lruItem)
 		if !isSizeMismatch(size, item.size) {
 			var f *os.File
 			f, err = os.Open(blobPath)
@@ -591,9 +585,8 @@ func (c *Cache) Contains(kind cache.EntryKind, hash string, size int64) (bool, i
 	key := cache.Key(kind, hash)
 
 	c.mu.Lock()
-	val, exists := c.lru.Get(key)
+	item, exists := c.lru.Get(key)
 	if exists {
-		item := val.(*lruItem)
 		foundSize = item.size
 	}
 	c.mu.Unlock()
diff --git a/cache/disk/lru.go b/cache/disk/lru.go
index f63b1cd..67165b5 100644
--- a/cache/disk/lru.go
+++ b/cache/disk/lru.go
@@ -39,28 +39,12 @@ type sizedItem interface {
 type Key interface{}
 
 // EvictCallback is the type of callbacks that are invoked when items are evicted.
-type EvictCallback func(key Key, value sizedItem)
+type EvictCallback func(key Key, value lruItem)
 
 // SizedLRU is an LRU cache that will keep its total size below maxSize by evicting
 // items.
 // SizedLRU is not thread-safe.
-type SizedLRU interface {
-	Add(key Key, value sizedItem) (ok bool)
-	Get(key Key) (value sizedItem, ok bool)
-	Remove(key Key)
-	Len() int
-	TotalSize() int64
-	ReservedSize() int64
-	MaxSize() int64
-
-	// Reserve some amount of cache space.
-	Reserve(size int64) (ok bool, err error)
-
-	// Release some amount of reserved cache space.
-	Unreserve(size int64) error
-}
-
-type sizedLRU struct {
+type SizedLRU struct {
 	// Eviction double-linked list. Most recently accessed elements are at the front.
 	ll *list.List
 	// Map to access the items in O(1) time
@@ -80,12 +64,12 @@ type sizedLRU struct {
 
 type entry struct {
 	key   Key
-	value sizedItem
+	value lruItem
 }
 
-// NewSizedLRU returns a new sizedLRU cache
+// NewSizedLRU returns a new SizedLRU cache
 func NewSizedLRU(maxSize int64, onEvict EvictCallback) SizedLRU {
-	return &sizedLRU{
+	return SizedLRU{
 		maxSize: maxSize,
 		ll:      list.New(),
 		cache:   make(map[interface{}]*list.Element),
@@ -96,22 +80,22 @@ func NewSizedLRU(maxSize int64, onEvict EvictCallback) SizedLRU {
 // Add adds a (key, value) to the cache, evicting items as necessary. Add returns false (
 // and does not add the item) if the item size is larger than the maximum size of the cache,
 // or it cannot be added to the cache because too much space is reserved.
-func (c *sizedLRU) Add(key Key, value sizedItem) (ok bool) {
-	if value.Size() > c.maxSize {
+func (c *SizedLRU) Add(key Key, value lruItem) (ok bool) {
+	if value.size > c.maxSize {
 		return false
 	}
 
 	var sizeDelta int64
 	if ee, ok := c.cache[key]; ok {
-		sizeDelta = value.Size() - ee.Value.(*entry).value.Size()
+		sizeDelta = value.size - ee.Value.(*entry).value.size
 		if c.reservedSize+sizeDelta > c.maxSize {
 			return false
 		}
 		c.ll.MoveToFront(ee)
-		counterOverwrittenBytes.Add(float64(ee.Value.(*entry).value.Size()))
+		counterOverwrittenBytes.Add(float64(ee.Value.(*entry).value.size))
 		ee.Value.(*entry).value = value
 	} else {
-		sizeDelta = value.Size()
+		sizeDelta = value.size
 		if c.reservedSize+sizeDelta > c.maxSize {
 			return false
 		}
@@ -136,7 +120,7 @@ func (c *sizedLRU) Add(key Key, value sizedItem) (ok bool) {
 }
 
 // Get looks up a key in the cache
-func (c *sizedLRU) Get(key Key) (value sizedItem, ok bool) {
+func (c *SizedLRU) Get(key Key) (value lruItem, ok bool) {
 	if ele, hit := c.cache[key]; hit {
 		c.ll.MoveToFront(ele)
 		return ele.Value.(*entry).value, true
@@ -146,7 +130,7 @@ func (c *sizedLRU) Get(key Key) (value sizedItem, ok bool) {
 }
 
 // Remove removes a (key, value) from the cache
-func (c *sizedLRU) Remove(key Key) {
+func (c *SizedLRU) Remove(key Key) {
 	if ele, hit := c.cache[key]; hit {
 		c.removeElement(ele)
 		gaugeCacheSizeBytes.Set(float64(c.currentSize))
@@ -154,19 +138,19 @@ func (c *sizedLRU) Remove(key Key) {
 }
 
 // Len returns the number of items in the cache
-func (c *sizedLRU) Len() int {
+func (c *SizedLRU) Len() int {
 	return len(c.cache)
 }
 
-func (c *sizedLRU) TotalSize() int64 {
+func (c *SizedLRU) TotalSize() int64 {
 	return c.currentSize
 }
 
-func (c *sizedLRU) ReservedSize() int64 {
+func (c *SizedLRU) ReservedSize() int64 {
 	return c.reservedSize
 }
 
-func (c *sizedLRU) MaxSize() int64 {
+func (c *SizedLRU) MaxSize() int64 {
 	return c.maxSize
 }
 
@@ -187,7 +171,7 @@ func sumLargerThan(a, b, c int64) bool {
 
 var errReservation = errors.New("internal reservation error")
 
-func (c *sizedLRU) Reserve(size int64) (bool, error) {
+func (c *SizedLRU) Reserve(size int64) (bool, error) {
 	if size == 0 {
 		return true, nil
 	}
@@ -218,7 +202,7 @@ func (c *sizedLRU) Reserve(size int64) (bool, error) {
 	return true, nil
 }
 
-func (c *sizedLRU) Unreserve(size int64) error {
+func (c *SizedLRU) Unreserve(size int64) error {
 	if size == 0 {
 		return nil
 	}
@@ -240,12 +224,12 @@ func (c *sizedLRU) Unreserve(size int64) error {
 	return nil
 }
 
-func (c *sizedLRU) removeElement(e *list.Element) {
+func (c *SizedLRU) removeElement(e *list.Element) {
 	c.ll.Remove(e)
 	kv := e.Value.(*entry)
 	delete(c.cache, kv.key)
-	c.currentSize -= kv.value.Size()
-	counterEvictedBytes.Add(float64(kv.value.Size()))
+	c.currentSize -= kv.value.size
+	counterEvictedBytes.Add(float64(kv.value.size))
 
 	if c.onEvict != nil {
 		c.onEvict(kv.key, kv.value)


Test patch:
diff --git a/cache/disk/lru_test.go b/cache/disk/lru_test.go
index d3638cb..f482fd3 100644
--- a/cache/disk/lru_test.go
+++ b/cache/disk/lru_test.go
@@ -1,21 +1,11 @@
 package disk
 
 import (
-	"fmt"
 	"math"
 	"reflect"
 	"testing"
 )
 
-type testSizedItem struct {
-	s       int64
-	payload string
-}
-
-func (it *testSizedItem) Size() int64 {
-	return it.s
-}
-
 func checkSizeAndNumItems(t *testing.T, lru SizedLRU, expSize int64, expNum int) {
 	currentSize := lru.TotalSize()
 	if currentSize != expSize {
@@ -46,8 +36,8 @@ func TestBasics(t *testing.T) {
 
 	// Add an item
 	aKey := "akey"
-	anItem := testSizedItem{5, "hello"}
-	ok = lru.Add(aKey, &anItem)
+	anItem := lruItem{size: 5}
+	ok = lru.Add(aKey, anItem)
 	if !ok {
 		t.Fatalf("Add: failed inserting item")
 	}
@@ -56,7 +46,7 @@ func TestBasics(t *testing.T) {
 	if !getOk {
 		t.Fatalf("Get: failed getting item")
 	}
-	if *getItem.(*testSizedItem) != anItem {
+	if getItem.size != anItem.size {
 		t.Fatalf("Get: got a different item back")
 	}
 
@@ -70,7 +60,7 @@ func TestBasics(t *testing.T) {
 func TestEviction(t *testing.T) {
 	// Keep track of evictions using the callback
 	var evictions []int
-	onEvict := func(key Key, value sizedItem) {
+	onEvict := func(key Key, value lruItem) {
 		evictions = append(evictions, key.(int))
 	}
 
@@ -95,8 +85,8 @@ func TestEviction(t *testing.T) {
 	var expectedEvictions []int
 
 	for i, thisExpected := range expectedSizesNumItems {
-		item := testSizedItem{int64(i), fmt.Sprintf("%d", i)}
-		ok := lru.Add(i, &item)
+		item := lruItem{size: int64(i)}
+		ok := lru.Add(i, item)
 		if !ok {
 			t.Fatalf("Add: failed adding %d", i)
 		}
@@ -114,7 +104,7 @@ func TestRejectBigItem(t *testing.T) {
 	// Bounded caches should reject big items
 	lru := NewSizedLRU(10, nil)
 
-	ok := lru.Add("hello", &testSizedItem{11, "hello"})
+	ok := lru.Add("hello", lruItem{size: 11})
 	if ok {
 		t.Fatalf("Add succeeded, expected it to fail")
 	}
@@ -123,10 +113,10 @@ func TestRejectBigItem(t *testing.T) {
 }
 
 func TestReserveZeroAlwaysPossible(t *testing.T) {
-	largeItem := testSizedItem{math.MaxInt64, "pretend large item"}
+	largeItem := lruItem{size: math.MaxInt64}
 
 	lru := NewSizedLRU(math.MaxInt64, nil)
-	lru.Add("foo", &largeItem)
+	lru.Add("foo", largeItem)
 	ok, err := lru.Reserve(0)
 	if err != nil {
 		t.Fatal(err)
@@ -266,7 +256,7 @@ func TestAddWithSpaceReserved(t *testing.T) {
 		t.Fatalf("Expected to be able to reserve 1")
 	}
 
-	ok = lru.Add("hello", &testSizedItem{2, "hello"})
+	ok = lru.Add("hello", lruItem{size: 2})
 	if ok {
 		t.Fatal("Expected to not be able to add item with size 2")
 	}
@@ -276,7 +266,7 @@ func TestAddWithSpaceReserved(t *testing.T) {
 		t.Fatal("Expected to be able to unreserve 1:", err)
 	}
 
-	ok = lru.Add("hello", &testSizedItem{2, "hello"})
+	ok = lru.Add("hello", lruItem{size: 2})
 	if !ok {
 		t.Fatal("Expected to be able to add item with size 2")
 	}