Merge pull request #26 from square/efficient-collection

Load collections more efficiently
square · Mar 18, 2024 · 04cdc1a · 04cdc1a
2 parents d9aceac + 9b0472d
commit 04cdc1a
Show file tree

Hide file tree

Showing 5 changed files with 114 additions and 59 deletions.
diff --git a/src/KnownMiss.php b/src/KnownMiss.php
@@ -0,0 +1,26 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Square\TTCache;
+
+/**
+ * Marker when using ->load() to remember that this was already checked and there is no need to check again
+ */
+class KnownMiss extends TaggedValue
+{
+    public function __construct()
+    {
+        parent::__construct(null, []);
+    }
+
+    public function hasError(): bool
+    {
+        return false;
+    }
+
+    public function error(): ?\Throwable
+    {
+        return null;
+    }
+}
diff --git a/src/TTCache.php b/src/TTCache.php
@@ -75,20 +75,27 @@ public function remember(string $key, callable $cb, array $tags = [], ?int $ttl
         $isRoot = $this->initTree();
 
         // Retrieve it from local cache if possible
+        $isKnownMiss = false;
         if ($this->tree->inCache($hkey)) {
             $r = $this->tree->getFromCache($hkey);
-            $this->tree->child($r->tags);
-            $this->resetTree($isRoot);
+            if ($r instanceof KnownMiss) {
+                $isKnownMiss = true;
+            } else {
+                $this->tree->child($r->tags);
+                $this->resetTree($isRoot);
 
-            return Result::fromTaggedValue($r, true);
+                return Result::fromTaggedValue($r, true);
+            }
         }
 
-        $r = $this->cache->get($hkey);
-        if ($r->value()) {
-            $this->tree->child($r->value()->tags);
-            $this->resetTree($isRoot);
+        if (! $isKnownMiss) { // If it's a known miss, don't bother checking the cache
+            $r = $this->cache->get($hkey);
+            if ($r->value()) {
+                $this->tree->child($r->value()->tags);
+                $this->resetTree($isRoot);
 
-            return Result::fromTaggedValue($r->value(), true)->withError($r->error());
+                return Result::fromTaggedValue($r->value(), true)->withError($r->error());
+            }
         }
 
         ['readonly' => $roCache, 'taghashes' => $tagHashes] = $this->cache->fetchOrMakeTagHashes($htags, $ttl);
@@ -204,6 +211,11 @@ public function load(array $keys): LoadResult
         }
         $this->tree->addToCache($validValuesResult->value());
 
+        // Add known misses to the local cache for the keys that were not found
+        $missingKeys = array_diff($hkeys, array_keys($validValuesResult->value()));
+        $missingKeys = array_combine($missingKeys, array_fill(0, count($missingKeys), new KnownMiss));
+        $this->tree->addToCache($missingKeys);
+
         return new LoadResult($loadedKeys, $keys, $validValuesResult->error());
     }
 

diff --git a/tests/KeyTracker.php b/tests/KeyTracker.php
@@ -22,7 +22,7 @@ public function get(string $key, mixed $default = null): mixed
         return $this->inner->get($key, $default);
     }
 
-    public function set(string $key, mixed $value, int|\DateInterval $ttl = null): bool
+    public function set(string $key, mixed $value, int|\DateInterval|null $ttl = null): bool
     {
         return $this->inner->set($key, $value, $ttl);
     }
@@ -34,10 +34,14 @@ public function delete(string $key): bool
 
     public function getMultiple(iterable $keys, mixed $default = null): iterable
     {
+        foreach ($keys as $key) {
+            $this->requestedKeys[] = $key;
+        }
+
         return $this->inner->getMultiple($keys);
     }
 
-    public function setMultiple(iterable $values, int|\DateInterval $ttl = null): bool
+    public function setMultiple(iterable $values, int|\DateInterval|null $ttl = null): bool
     {
         return $this->inner->setMultiple($values, $ttl);
     }
@@ -46,4 +50,9 @@ public function deleteMultiple(iterable $keys): bool
     {
         return $this->inner->deleteMultiple($keys);
     }
+
+    public function getRequestedKeys(): array
+    {
+        return array_filter($this->requestedKeys, fn ($key) => ! str_starts_with($key, 't-'));
+    }
 }
diff --git a/tests/MemcacheTTCacheTest.php b/tests/MemcacheTTCacheTest.php
@@ -7,30 +7,45 @@
 
 class MemcacheTTCacheTest extends TTCacheTest
 {
-    /**
-     * @var Memcached
-     */
     protected Memcached $mc;
 
     public function getTTCache(): TTCache
     {
-        if (!isset($this->mc)) {
+        if (! isset($this->mc)) {
             $this->mc = new Memcached;
             $this->mc->addServers([['memcached', 11211]]);
             $this->mc->flush();
         }
 
         $store = new ShardedMemcachedStore($this->mc);
         $store->setShardingKey('hello');
-        return new TTCache($store);
+
+        return new TTCache($store, fn ($a) => $a);
+    }
+
+    public function getTTCacheWithKeyTracker(): TTCache
+    {
+        if (! isset($this->mc)) {
+            $this->mc = new Memcached;
+            $this->mc->addServers([['memcached', 11211]]);
+            $this->mc->flush();
+        }
+
+        $store = new ShardedMemcachedStore($this->mc);
+        $store->setShardingKey('hello');
+
+        $this->keyTracker = new KeyTracker($store);
+
+        return new TTCache($this->keyTracker, fn ($a) => $a);
     }
 
     public function getBogusTTCache(): TTCache
     {
         $this->mc = new Memcached;
         $store = new ShardedMemcachedStore($this->mc);
         $store->setShardingKey('hello');
-        return new TTCache($store);
+
+        return new TTCache($store, fn ($a) => $a);
     }
 
     public function tearDown(): void

diff --git a/tests/TTCacheTest.php b/tests/TTCacheTest.php
@@ -7,8 +7,6 @@
 use Memcached;
 use PHPUnit\Framework\TestCase;
 use Square\TTCache\ReturnDirective\BypassCache;
-use Square\TTCache\Store\CacheStoreInterface;
-use Square\TTCache\Store\TaggedStore;
 use Square\TTCache\Tags\HeritableTag;
 use Square\TTCache\Tags\ShardingTag;
 
@@ -18,13 +16,17 @@ abstract class TTCacheTest extends TestCase
 
     protected Memcached $mc;
 
+    protected KeyTracker $keyTracker;
+
     public function setUp(): void
     {
         $this->tt = $this->getTTCache();
     }
 
     abstract public function getTTCache(): TTCache;
 
+    abstract public function getTTCacheWithKeyTracker(): TTCache;
+
     /**
      * Returns a TTCache implementation which will fail connecting to the
      * cache store
@@ -295,7 +297,7 @@ public function caching_from_a_cached_value_still_applies_inner_tags()
         });
 
         $this->assertSame($built()->value(), 'hello dear world!');
-        $this->assertSame($built()->tags(), ['t-'.md5('uppertag'), 't-'.md5('sub:1')]);
+        $this->assertSame($built()->tags(), ['t-uppertag', 't-sub:1']);
         $this->assertSame($counter->get(), 2, 'expected first call to call all 4 levels.');
 
         $this->tt->clearTags(('uppertag'));
@@ -304,15 +306,15 @@ public function caching_from_a_cached_value_still_applies_inner_tags()
         // Only 1 level of re-computing happened. 'sub' was retrieved from cache
         // but its tags still got applied to the upper level
         $this->assertSame($built()->value(), 'hello dear world!');
-        $this->assertSame($built()->tags(), ['t-'.md5('uppertag'), 't-'.md5('sub:1')]);
+        $this->assertSame($built()->tags(), ['t-uppertag', 't-sub:1']);
         $this->assertSame($counter->get(), 1, 'expected first call to call all 4 levels.');
 
         $this->tt->clearTags(('sub:1'));
         $counter->reset();
 
         // So now when clearing the sub tag, we go 2 levels deep
         $this->assertSame($built()->value(), 'hello dear world!');
-        $this->assertSame($built()->tags(), ['t-'.md5('uppertag'), 't-'.md5('sub:1')]);
+        $this->assertSame($built()->tags(), ['t-uppertag', 't-sub:1']);
         $this->assertSame($counter->get(), 2, 'expected first call to call all 4 levels.');
     }
 
@@ -553,29 +555,7 @@ public function retrieving_parts_of_a_collection_still_applies_all_tags()
             new BlogPost('nop', 'Learn Go the curved way', '...'),
         ];
 
-        /*
-         * Very hacky, but we are using reflection here to add a layer around
-         * the inner-most CacheStoreInterface that tracks the keys requested by the TaggedStore
-         * via $store->get(...). e.g.:
-         */
-        $reflClass = new \ReflectionClass(TTCache::class);
-        $reflProperty = $reflClass->getProperty('cache');
-        $reflProperty->setAccessible(true);
-        // This is the TaggedStore instance on TTCache. This would contain the CacheStoreInterface we want to track.
-        $taggedStore = $reflProperty->getValue($this->tt);
-        $reflClass = new \ReflectionClass(TaggedStore::class);
-        $reflProperty = $reflClass->getProperty('cache');
-        $reflProperty->setAccessible(true);
-        /**
-         * This is the inner-most CacheStoreInterface.
-         *
-         * @var CacheStoreInterface $origStore
-         */
-        $origStore = $reflProperty->getValue($taggedStore);
-
-        // We will add the layer around it, and set it back to the TaggedStore.
-        $store = new KeyTracker($origStore);
-        $reflProperty->setValue($taggedStore, $store);
+        $this->tt = $this->getTTCacheWithKeyTracker();
 
         $resultReaderStub = (object) [
             'result' => null,
@@ -611,7 +591,7 @@ public function retrieving_parts_of_a_collection_still_applies_all_tags()
             'k-'.$this->hash('Square\TTCache\TTCacheTest:blog-collection:ghi'),
             'k-'.$this->hash('Square\TTCache\TTCacheTest:blog-collection:klm'),
             'k-'.$this->hash('Square\TTCache\TTCacheTest:blog-collection:nop'),
-        ], $store->requestedKeys);
+        ], $this->keyTracker->getRequestedKeys());
 
         $this->assertInstanceOf(LoadResult::class, $resultReaderStub->result);
         $this->assertEmpty($resultReaderStub->result->loadedKeys());
@@ -623,19 +603,19 @@ public function retrieving_parts_of_a_collection_still_applies_all_tags()
             'Square\TTCache\TTCacheTest:blog-collection:nop',
         ], $resultReaderStub->result->missingKeys());
 
-        // When we call `built()` again, all the data should be pre-loaded and therefore come without talking to MC
+        // When we call `built()` again, all the data should be pre-loaded and therefore come with a single call to MC
         $resultReaderStub->result = null;
-        $store->requestedKeys = [];
+        $this->keyTracker->requestedKeys = [];
         $built();
         $this->assertSame([
             'k-'.$this->hash('full-collection'),
-        ], $store->requestedKeys);
+        ], $this->keyTracker->getRequestedKeys());
         $this->assertNull($resultReaderStub->result);
 
         // Clear tag for "abc" and change the title for "abc"
         $this->tt->clearTags('post:'.$coll[0]->id);
         $resultReaderStub->result = null;
-        $store->requestedKeys = [];
+        $this->keyTracker->requestedKeys = [];
         $coll[0]->title = 'Learn PHP the straight way';
         $this->assertSame([
             '<h1>Learn PHP the straight way</h1><hr /><div>...</div>',
@@ -644,10 +624,6 @@ public function retrieving_parts_of_a_collection_still_applies_all_tags()
             '<h1>Learn Rust the curved way</h1><hr /><div>...</div>',
             '<h1>Learn Go the curved way</h1><hr /><div>...</div>',
         ], $built());
-        $this->assertSame([
-            'k-'.$this->hash('full-collection'),
-            'k-'.$this->hash('Square\TTCache\TTCacheTest:blog-collection:abc'),
-        ], $store->requestedKeys);
         $this->assertInstanceOf(LoadResult::class, $resultReaderStub->result);
         $this->assertEquals([
             1 => 'Square\TTCache\TTCacheTest:blog-collection:def',
@@ -661,7 +637,7 @@ public function retrieving_parts_of_a_collection_still_applies_all_tags()
 
         // Newly cached value still contains all the tags. So clearing by another tag will also work.
         $this->tt->clearTags('post:'.$coll[1]->id);
-        $store->requestedKeys = [];
+        $this->keyTracker->requestedKeys = [];
         $resultReaderStub->result = null;
         $coll[1]->title = 'Learn Python the straight way';
         $this->assertEquals([
@@ -671,10 +647,6 @@ public function retrieving_parts_of_a_collection_still_applies_all_tags()
             '<h1>Learn Rust the curved way</h1><hr /><div>...</div>',
             '<h1>Learn Go the curved way</h1><hr /><div>...</div>',
         ], $built());
-        $this->assertSame([
-            'k-'.$this->hash('full-collection'),
-            'k-'.$this->hash('Square\TTCache\TTCacheTest:blog-collection:def'),
-        ], $store->requestedKeys);
         $this->assertInstanceOf(LoadResult::class, $resultReaderStub->result);
         $this->assertEquals([
             0 => 'Square\TTCache\TTCacheTest:blog-collection:abc',
@@ -721,6 +693,27 @@ public function collection_result_still_gets_computed_when_cache_is_down()
         });
     }
 
+    /**
+     * When we ->load() a collection, if some of the keys were not found on the server, we should remember that
+     * and not go check the server agin for those keys
+     *
+     * @group debug
+     *
+     * @test
+     */
+    public function collection_non_loaded_members_dont_check_remote_cache_again()
+    {
+        $this->tt = $this->getTTCacheWithKeyTracker();
+        $this->tt->wrap([], function () {
+            $this->tt->load(['hello1', 'hello2', 'hello3']);
+
+            $this->tt->remember('hello1', fn () => 1);
+            $this->tt->remember('hello2', fn () => 2);
+            $this->tt->remember('hello3', fn () => 3);
+        });
+        $this->assertEquals(3, count($this->keyTracker->requestedKeys));
+    }
+
     /**
      * @test
      */
@@ -736,6 +729,6 @@ public function tag_hashes_match()
      */
     protected function hash(string $key): string
     {
-        return md5($key);
+        return $key;
     }
 }