From b8efb8eaea8a5b7ba00488c0d5c0a3232ede162c Mon Sep 17 00:00:00 2001 From: Pawit Pornkitprasan Date: Sun, 3 Oct 2021 00:09:33 +0700 Subject: [PATCH] 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 | 22 +++++++++++++++------- fs/cachefs/cache_test.go | 32 ++++++++++++++++++-------------- fs/cachefs/cachefs.go | 13 +++++-------- 3 files changed, 38 insertions(+), 29 deletions(-) diff --git a/fs/cachefs/cache.go b/fs/cachefs/cache.go index 481e7454a..65fafb81a 100644 --- a/fs/cachefs/cache.go +++ b/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) } diff --git a/fs/cachefs/cache_test.go b/fs/cachefs/cache_test.go index c3a832072..86f82d9d0 100644 --- a/fs/cachefs/cache_test.go +++ b/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() { diff --git a/fs/cachefs/cachefs.go b/fs/cachefs/cachefs.go index 5147513b2..c9a75f4cf 100644 --- a/fs/cachefs/cachefs.go +++ b/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 {