cachefs: make cache read O(1) instead of O(n) (#1341)

* cachefs: make cache read O(1) instead of O(n)

Move the wrapping of fs.Entry before caching rather than doing
it everytime we try to read from the cache.

fs/cachefs/cache.go

@@ -80,13 +80,16 @@ func (c *Cache) remove(e *cacheEntry) {
 // Loader provides data to be stored in the cache.
 type Loader func(ctx context.Context) (fs.Entries, error)
 
+// EntryWrapper allows an fs.Entry to be modified before inserting into the cache.
+type EntryWrapper func(entry fs.Entry) fs.Entry
+
 // Readdir reads the contents of a provided directory using ObjectID of a directory (if any) to cache
 // the results.
-func (c *Cache) Readdir(ctx context.Context, d fs.Directory) (fs.Entries, error) {
+func (c *Cache) Readdir(ctx context.Context, d fs.Directory, w EntryWrapper) (fs.Entries, error) {
 	if h, ok := d.(object.HasObjectID); ok {
 		cacheID := string(h.ObjectID())
 
-		return c.getEntries(ctx, cacheID, dirCacheExpiration, d.Readdir)
+		return c.getEntries(ctx, cacheID, dirCacheExpiration, d.Readdir, w)
 	}
 
 	// nolint:wrapcheck
@@ -118,7 +121,7 @@ func (c *Cache) getEntriesFromCacheLocked(ctx context.Context, id string) fs.Ent
 
 // getEntries consults the cache and either retrieves the contents of directory listing from the cache
 // or invokes the provides callback and adds the results to cache.
-func (c *Cache) getEntries(ctx context.Context, id string, expirationTime time.Duration, cb Loader) (fs.Entries, error) {
+func (c *Cache) getEntries(ctx context.Context, id string, expirationTime time.Duration, cb Loader, w EntryWrapper) (fs.Entries, error) {
 	if c == nil {
 		return cb(ctx)
 	}
@@ -139,21 +142,26 @@ func (c *Cache) getEntries(ctx context.Context, id string, expirationTime time.D
 		return nil, err
 	}
 
-	if len(raw) > c.maxDirectoryEntries {
+	wrapped := make(fs.Entries, len(raw))
+	for i, entry := range raw {
+		wrapped[i] = w(entry)
+	}
+
+	if len(wrapped) > c.maxDirectoryEntries {
 		// no point caching since it would not fit anyway, just return it.
-		return raw, nil
+		return wrapped, nil
 	}
 
 	entry := &cacheEntry{
 		id:          id,
-		entries:     raw,
+		entries:     wrapped,
 		expireAfter: clock.Now().Add(expirationTime),
 	}
 
 	c.addToHead(entry)
 	c.data[id] = entry
 
-	c.totalDirectoryEntries += len(raw)
+	c.totalDirectoryEntries += len(wrapped)
 	for c.totalDirectoryEntries > c.maxDirectoryEntries || len(c.data) > c.maxDirectories {
 		c.removeEntryLocked(c.tail)
 	}

fs/cachefs/cache_test.go

@@ -24,6 +24,10 @@ type cacheSource struct {
 	callCounter map[string]int
 }
 
+func identityWrapper(e fs.Entry) fs.Entry {
+	return e
+}
+
 func (cs *cacheSource) get(id string) func(ctx context.Context) (fs.Entries, error) {
 	return func(context.Context) (fs.Entries, error) {
 		cs.callCounter[id]++
@@ -187,57 +191,57 @@ func TestCache(t *testing.T) {
 	cv.verifyCacheOrdering(t)
 
 	// fetch id1
-	_, _ = c.getEntries(ctx, id1, expirationTime, cs.get(id1))
+	_, _ = c.getEntries(ctx, id1, expirationTime, cs.get(id1), identityWrapper)
 	cv.verifyCacheMiss(t, id1)
 	cv.verifyCacheOrdering(t, id1)
 
 	// fetch id1 again - cache hit, no change
-	_, _ = c.getEntries(ctx, id1, expirationTime, cs.get(id1))
+	_, _ = c.getEntries(ctx, id1, expirationTime, cs.get(id1), identityWrapper)
 	cv.verifyCacheHit(t, id1)
 	cv.verifyCacheOrdering(t, id1)
 
 	// fetch id2
-	_, _ = c.getEntries(ctx, id2, expirationTime, cs.get(id2))
+	_, _ = c.getEntries(ctx, id2, expirationTime, cs.get(id2), identityWrapper)
 	cv.verifyCacheMiss(t, id2)
 	cv.verifyCacheOrdering(t, id2, id1)
 
 	// fetch id1 again - cache hit, id1 moved to the top of the LRU list
-	_, _ = c.getEntries(ctx, id1, expirationTime, cs.get(id1))
+	_, _ = c.getEntries(ctx, id1, expirationTime, cs.get(id1), identityWrapper)
 	cv.verifyCacheHit(t, id1)
 	cv.verifyCacheOrdering(t, id1, id2)
 
 	// fetch id2 again
-	_, _ = c.getEntries(ctx, id2, expirationTime, cs.get(id2))
+	_, _ = c.getEntries(ctx, id2, expirationTime, cs.get(id2), identityWrapper)
 	cv.verifyCacheHit(t, id2)
 	cv.verifyCacheOrdering(t, id2, id1)
 
 	// fetch id3
-	_, _ = c.getEntries(ctx, id3, expirationTime, cs.get(id3))
+	_, _ = c.getEntries(ctx, id3, expirationTime, cs.get(id3), identityWrapper)
 	cv.verifyCacheMiss(t, id3)
 	cv.verifyCacheOrdering(t, id3, id2, id1)
 
 	// fetch id4
-	_, _ = c.getEntries(ctx, id4, expirationTime, cs.get(id4))
+	_, _ = c.getEntries(ctx, id4, expirationTime, cs.get(id4), identityWrapper)
 	cv.verifyCacheMiss(t, id4)
 	cv.verifyCacheOrdering(t, id4, id3)
 
 	// fetch id1 again
-	_, _ = c.getEntries(ctx, id1, expirationTime, cs.get(id1))
+	_, _ = c.getEntries(ctx, id1, expirationTime, cs.get(id1), identityWrapper)
 	cv.verifyCacheMiss(t, id1)
 	cv.verifyCacheOrdering(t, id1, id4)
 
 	// fetch id5, it's a big one that expels all but one
-	_, _ = c.getEntries(ctx, id5, expirationTime, cs.get(id5))
+	_, _ = c.getEntries(ctx, id5, expirationTime, cs.get(id5), identityWrapper)
 	cv.verifyCacheMiss(t, id5)
 	cv.verifyCacheOrdering(t, id5, id1)
 
 	// fetch id6
-	_, _ = c.getEntries(ctx, id6, expirationTime, cs.get(id6))
+	_, _ = c.getEntries(ctx, id6, expirationTime, cs.get(id6), identityWrapper)
 	cv.verifyCacheMiss(t, id6)
 	cv.verifyCacheOrdering(t, id6)
 
 	// fetch id7
-	_, _ = c.getEntries(ctx, id7, expirationTime, cs.get(id7))
+	_, _ = c.getEntries(ctx, id7, expirationTime, cs.get(id7), identityWrapper)
 	cv.verifyCacheMiss(t, id7)
 	cv.verifyCacheOrdering(t, id6)
 }
@@ -264,7 +268,7 @@ func TestCacheGetEntriesLocking(t *testing.T) {
 	cs.setEntryCount(id1, 1)
 
 	// fetch non-existing entry, the loader will return an error
-	actualEs, err := c.getEntries(ctx, id1, expirationTime, cs.get("foo"))
+	actualEs, err := c.getEntries(ctx, id1, expirationTime, cs.get("foo"), identityWrapper)
 	if err == nil {
 		t.Fatal("Expected non-nil error when retrieving non-existing cache entry")
 	}
@@ -281,10 +285,10 @@ func TestCacheGetEntriesLocking(t *testing.T) {
 	}
 
 	// fetch id1
-	_, _ = c.getEntries(ctx, id1, expirationTime, cs.get(id1))
+	_, _ = c.getEntries(ctx, id1, expirationTime, cs.get(id1), identityWrapper)
 	cv.verifyCacheMiss(t, id1)
 	// fetch id1 again - cache hit, no change
-	_, _ = c.getEntries(ctx, id1, expirationTime, cs.get(id1))
+	_, _ = c.getEntries(ctx, id1, expirationTime, cs.get(id1), identityWrapper)
 	cv.verifyCacheHit(t, id1)
 	// cache must be unlocked and there should be no double unlock: See #132
 	if !lock.Unlocked() {

fs/cachefs/cachefs.go

@@ -9,7 +9,7 @@ import (
 
 // DirectoryCacher reads and potentially caches directory entries for a given directory.
 type DirectoryCacher interface {
-	Readdir(ctx context.Context, d fs.Directory) (fs.Entries, error)
+	Readdir(ctx context.Context, d fs.Directory, w EntryWrapper) (fs.Entries, error)
 }
 
 type cacheContext struct {
@@ -32,19 +32,16 @@ func (d *directory) Child(ctx context.Context, name string) (fs.Entry, error) {
 }
 
 func (d *directory) Readdir(ctx context.Context) (fs.Entries, error) {
-	entries, err := d.ctx.cacher.Readdir(ctx, d.Directory)
+	entries, err := d.ctx.cacher.Readdir(ctx, d.Directory, func(e fs.Entry) fs.Entry {
+		return wrapWithContext(e, d.ctx)
+	})
 	if err != nil {
 		// nolint:wrapcheck
 		return entries, err
 	}
 
-	wrapped := make(fs.Entries, len(entries))
-	for i, entry := range entries {
-		wrapped[i] = wrapWithContext(entry, d.ctx)
-	}
-
 	// nolint:wrapcheck
-	return wrapped, err
+	return entries, err
 }
 
 type file struct {