Merge pull request #35 from bigcommerce/lru

Fix LRU cache mechanism to evict least recently used instead of FIFO

Author: TomA-R

src/Reflection/ReflectionClassCache.php

@@ -27,10 +27,14 @@ public function __construct(int $maxSize)
      */
     public function put(ReflectionClass $reflection): void
     {
-        if (count($this->map) >= $this->maxSize) {
+        $className = $reflection->getName();
+        // if already present, remove it so we can re-insert it as most recently used item
+        if (isset($this->map[$className])) {
+            unset($this->map[$className]);
+        } elseif (count($this->map) >= $this->maxSize) {
             $this->evictOneObject();
         }
-        $this->map[$reflection->getName()] = $reflection;
+        $this->map[$className] = $reflection;
     }
 
     /**
@@ -40,7 +44,11 @@ public function put(ReflectionClass $reflection): void
     public function get(string $className): ?ReflectionClass
     {
         if (isset($this->map[$className])) {
-            return $this->map[$className];
+            // promote to most recently used item by re-inserting this to the end of the array
+            $reflection = $this->map[$className];
+            unset($this->map[$className]);
+            $this->map[$className] = $reflection;
+            return $reflection;
         }
         return null;
     }
@@ -57,7 +65,6 @@ public function count(): int
 
     private function evictOneObject(): void
     {
-        // Get the least recently used item (first in insertion order)
         $lruClassName = array_key_first($this->map);
         if ($lruClassName !== null) {
             unset($this->map[$lruClassName]);

tests/Reflection/ReflectionClassCacheTest.php

@@ -68,4 +68,44 @@ public function testCacheDoesNotExceedMaxSize(): void
 
         $this->assertEquals(2, $result);
     }
+
+    public function testGetPromotesToMostRecentlyUsedSoItDoesNotEvictNext(): void
+    {
+        $cache = new ReflectionClassCache(2);
+
+        $classA = DummyDependency::class;
+        $classB = DummyNoConstructor::class;
+        $classC = DummySimpleConstructor::class;
+
+        $cache->put(new ReflectionClass($classA));
+        $cache->put(new ReflectionClass($classB));
+
+        $this->assertNotNull($cache->get($classA));
+
+        $cache->put(new ReflectionClass($classC));
+
+        $this->assertTrue($cache->has($classA));
+        $this->assertFalse($cache->has($classB));
+        $this->assertTrue($cache->has($classC));
+    }
+
+    public function testPutRefreshesRecencySoReinsertedEntryIsNotEvicted(): void
+    {
+        $cache = new ReflectionClassCache(2);
+
+        $classA = DummyDependency::class;
+        $classB = DummyNoConstructor::class;
+        $classC = DummySimpleConstructor::class;
+
+        $cache->put(new ReflectionClass($classA));
+        $cache->put(new ReflectionClass($classB));
+
+        $cache->put(new ReflectionClass($classA));
+
+        $cache->put(new ReflectionClass($classC));
+
+        $this->assertTrue($cache->has($classA));
+        $this->assertFalse($cache->has($classB));
+        $this->assertTrue($cache->has($classC));
+    }
 }