From a5eab38753173145be6c67ba51b034fb791c4919 Mon Sep 17 00:00:00 2001 From: Jonathan Johnson Date: Wed, 5 Jul 2017 21:04:16 -0700 Subject: [PATCH 1/2] Prevent the same loader from being registered twice Registering an expensive loader like `class_exists` twice doubles the number of calls to the callable - this prevents that from happening. --- lib/Doctrine/Common/Annotations/AnnotationRegistry.php | 4 +++- .../Tests/Common/Annotations/AnnotationRegistryTest.php | 8 ++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/Doctrine/Common/Annotations/AnnotationRegistry.php b/lib/Doctrine/Common/Annotations/AnnotationRegistry.php index 13ceb6348..73eeb086e 100644 --- a/lib/Doctrine/Common/Annotations/AnnotationRegistry.php +++ b/lib/Doctrine/Common/Annotations/AnnotationRegistry.php @@ -110,7 +110,9 @@ static public function registerLoader($callable) if (!is_callable($callable)) { throw new \InvalidArgumentException("A callable is expected in AnnotationRegistry::registerLoader()."); } - self::$loaders[] = $callable; + if (!in_array($callable, self::$loaders)) { + self::$loaders[] = $callable; + } } /** diff --git a/tests/Doctrine/Tests/Common/Annotations/AnnotationRegistryTest.php b/tests/Doctrine/Tests/Common/Annotations/AnnotationRegistryTest.php index 36db2b535..b66ce7bbd 100644 --- a/tests/Doctrine/Tests/Common/Annotations/AnnotationRegistryTest.php +++ b/tests/Doctrine/Tests/Common/Annotations/AnnotationRegistryTest.php @@ -65,4 +65,12 @@ protected function getStaticField($class, $field) return $reflection->getValue(); } + + public function testRegisterLoaderTwiceOnlySavedOnce() + { + AnnotationRegistry::registerLoader('class_exists'); + AnnotationRegistry::registerLoader('class_exists'); + + self::assertEquals(['class_exists'], $this->getStaticField($this->class, 'loaders')); + } } \ No newline at end of file From c60993f20a0bd5432dd4dd1b896b697d340693d9 Mon Sep 17 00:00:00 2001 From: Jonathan Johnson Date: Fri, 14 Jul 2017 12:36:07 -0700 Subject: [PATCH 2/2] Cache hits and misses in the AnnotationRegistry --- .../Common/Annotations/AnnotationRegistry.php | 33 +++++++++++++-- .../Annotations/AnnotationRegistryTest.php | 41 +++++++++++++++++-- 2 files changed, 68 insertions(+), 6 deletions(-) diff --git a/lib/Doctrine/Common/Annotations/AnnotationRegistry.php b/lib/Doctrine/Common/Annotations/AnnotationRegistry.php index 73eeb086e..7c3c3fb33 100644 --- a/lib/Doctrine/Common/Annotations/AnnotationRegistry.php +++ b/lib/Doctrine/Common/Annotations/AnnotationRegistry.php @@ -43,6 +43,20 @@ final class AnnotationRegistry */ static private $loaders = array(); + /** + * An array of loaded classes + * + * @var array + */ + static private $loaded = array(); + + /** + * An array of classes which cannot be found + * + * @var array + */ + static private $unloadable = array(); + /** * @return void */ @@ -50,6 +64,8 @@ static public function reset() { self::$autoloadNamespaces = array(); self::$loaders = array(); + self::$loaded = array(); + self::$unloadable = array(); } /** @@ -110,9 +126,10 @@ static public function registerLoader($callable) if (!is_callable($callable)) { throw new \InvalidArgumentException("A callable is expected in AnnotationRegistry::registerLoader()."); } - if (!in_array($callable, self::$loaders)) { - self::$loaders[] = $callable; - } + // Reset our static cache now that we have a new loader to work with + self::$loaded = array(); + self::$unloadable = array(); + self::$loaders[] = $callable; } /** @@ -124,18 +141,26 @@ static public function registerLoader($callable) */ static public function loadAnnotationClass($class) { + if (isset(self::$loaded[$class])) { + return true; + } + if (isset(self::$unloadable[$class])) { + return false; + } foreach (self::$autoloadNamespaces AS $namespace => $dirs) { if (strpos($class, $namespace) === 0) { $file = str_replace("\\", DIRECTORY_SEPARATOR, $class) . ".php"; if ($dirs === null) { if ($path = stream_resolve_include_path($file)) { require $path; + self::$loaded[$class] = true; return true; } } else { foreach((array)$dirs AS $dir) { if (is_file($dir . DIRECTORY_SEPARATOR . $file)) { require $dir . DIRECTORY_SEPARATOR . $file; + self::$loaded[$class] = true; return true; } } @@ -145,9 +170,11 @@ static public function loadAnnotationClass($class) foreach (self::$loaders AS $loader) { if (call_user_func($loader, $class) === true) { + self::$loaded[$class] = true; return true; } } + self::$unloadable[$class] = true; return false; } } diff --git a/tests/Doctrine/Tests/Common/Annotations/AnnotationRegistryTest.php b/tests/Doctrine/Tests/Common/Annotations/AnnotationRegistryTest.php index b66ce7bbd..7fb2bd0e8 100644 --- a/tests/Doctrine/Tests/Common/Annotations/AnnotationRegistryTest.php +++ b/tests/Doctrine/Tests/Common/Annotations/AnnotationRegistryTest.php @@ -25,6 +25,8 @@ public function testReset() self::assertEmpty($this->getStaticField($this->class, 'autoloadNamespaces')); self::assertEmpty($this->getStaticField($this->class, 'loaders')); + self::assertEmpty($this->getStaticField($this->class, 'loaded')); + self::assertEmpty($this->getStaticField($this->class, 'unloadable')); } /** @@ -66,11 +68,44 @@ protected function getStaticField($class, $field) return $reflection->getValue(); } - public function testRegisterLoaderTwiceOnlySavedOnce() + public function testStopCallingLoadersIfClassIsNotFound() { + AnnotationRegistry::reset(); + $i = 0; + $autoLoader = function($annotation) use (&$i) { + $i++; + return false; + }; + AnnotationRegistry::registerLoader($autoLoader); + AnnotationRegistry::loadAnnotationClass('unloadableClass'); + AnnotationRegistry::loadAnnotationClass('unloadableClass'); + AnnotationRegistry::loadAnnotationClass('unloadableClass'); + $this->assertEquals(1, $i, 'Autoloader should only be called once'); + } + + public function testStopCallingLoadersAfterClassIsFound() + { + AnnotationRegistry::reset(); + $i = 0; + $autoLoader = function($annotation) use (&$i) { + $i++; + return true; + }; + AnnotationRegistry::registerLoader($autoLoader); + AnnotationRegistry::loadAnnotationClass(self::class); + AnnotationRegistry::loadAnnotationClass(self::class); + AnnotationRegistry::loadAnnotationClass(self::class); + $this->assertEquals(1, $i, 'Autoloader should only be called once'); + } + + public function testAddingANewLoaderClearsTheCache() + { + AnnotationRegistry::reset(); AnnotationRegistry::registerLoader('class_exists'); + AnnotationRegistry::loadAnnotationClass(self::class); + AnnotationRegistry::loadAnnotationClass('unloadableClass'); AnnotationRegistry::registerLoader('class_exists'); - - self::assertEquals(['class_exists'], $this->getStaticField($this->class, 'loaders')); + self::assertEmpty($this->getStaticField($this->class, 'loaded')); + self::assertEmpty($this->getStaticField($this->class, 'unloadable')); } } \ No newline at end of file