#135 optimise multiple class load attempts in AnnotationRegistry
#137
Conversation
Registering an expensive loader like `class_exists` twice doubles the number of calls to the callable - this prevents that from happening.
…ust be using the internal autoloader
… check is replaced by hint
Ocramius
changed the title
AnnotationRegistry
|
Urgh, premature merge due to CLI mistake on my side. @lcobucci still, if you can, give it a review... |
| * @return void | ||
| * An array of classes which cannot be found | ||
| * | ||
| * @var null[] indexed by class name |
There was a problem hiding this comment.
Yeah, that's intentional, as a boolean can misrepresent two states, while here we just want to represent whether a key exists or not
| $file = str_replace("\\", DIRECTORY_SEPARATOR, $class) . ".php"; | ||
| if (\strpos($class, $namespace) === 0) { | ||
| $file = \str_replace('\\', \DIRECTORY_SEPARATOR, $class) . '.php'; | ||
|
|
||
| if ($dirs === null) { | ||
| if ($path = stream_resolve_include_path($file)) { | ||
| require $path; | ||
| return true; | ||
| } | ||
| } else { | ||
| foreach((array)$dirs AS $dir) { |
There was a problem hiding this comment.
Add a space after the casting ((array) $dirs)
| return true; | ||
| } | ||
| } | ||
|
|
||
| self::$failedToAutoload[$class] = null; |
There was a problem hiding this comment.
Any reason for setting the index instead pushing a new item to the array? It would never be duplicated anyway (since the check is done at the top).
Of course this is a micro optimisation but it might be worth it (also an array with only null values is a bit weird IMO)
There was a problem hiding this comment.
The lookup can happen a lot of times (thousands according to the original report), so I'd rather keep it indexed
| AnnotationRegistry::loadAnnotationClass('unloadableClass'); | ||
| AnnotationRegistry::loadAnnotationClass('unloadableClass'); | ||
| AnnotationRegistry::loadAnnotationClass('unloadableClass'); | ||
| $this->assertEquals(1, $i, 'Autoloader should only be called once'); |
| $i = 0; | ||
| $autoLoader = function($annotation) use (&$i, $className) { | ||
| eval('class ' . $className . ' {}'); | ||
| $i++; |
| AnnotationRegistry::loadAnnotationClass($className); | ||
| AnnotationRegistry::loadAnnotationClass($className); | ||
| AnnotationRegistry::loadAnnotationClass($className); | ||
| $this->assertEquals(1, $i, 'Autoloader should only be called once'); |
| { | ||
| $failures = 0; | ||
| $failingLoader = function (string $annotation) use (& $failures) : bool { | ||
| $failures += 1; |
There was a problem hiding this comment.
++$failures (to be consistent 😜)
| { | ||
| $failures = 0; | ||
| $failingLoader = function (string $annotation) use (& $failures) : bool { | ||
| $failures += 1; |
There was a problem hiding this comment.
++$failures (to be consistent 😜)
| AnnotationRegistry::loadAnnotationClass('unloadableClass'); | ||
|
|
||
| self::assertSame(2, $failures); | ||
| } | ||
| } |
|
@lcobucci changes applied on |
This PR includes #135 as well as a number of fixes around the
AnnotationRegistryclass (which is mostly a horror show from the pre-composer era).