Prevent the same loader from being registered twice #135
Conversation
Registering an expensive loader like `class_exists` twice doubles the number of calls to the callable - this prevents that from happening.
|
@jrjohnson shouldn't the loader simply stop dispatching further autoloaders if Having double autoloaders may actually be intentional on the user side in some scenarios, especially for changing order of autoloaders, so this patch in particular is invalid in my opinion. |
|
The problem isn't when there is a hit, it is when there is a miss. Something like How would you change the order of autoloaders by registering them twice? If you have I'm happy to pursue another path - for example I could add a static attribute that would enable this de-duplicating behavior - but I'm having trouble envisioning a scenario where that would be necessary. |
|
@jrjohnson here's what we can do, in my opinion (and would even be faster than preventing double autoloader registration):
That would be simple and effective. A testing strategy would be to create a mock autoloader and a class with annotations, then trying to load a non-registered annotation two times and verifying that it is not triggering autoloading after the first failure. |
|
@Ocramius thanks, I will get started on that. Is there any reason not to cache the successes as well while I'm at it? |
|
@Ocramius I profiled storing the misses in a static var and there is a significant performance penalty over my original change to prevent the double autoload: I think this is due to having to dump the cache when a new loader is registered. This means that the loaders still stack up and the cache is only hit a few times. I also tried caching the hits, but it wasn't any faster. This is how I wrote the change you asked for (with some truncation). static public function loadAnnotationClass($class)
{
if (in_array($class, self::$unloadable)) {
return false;
}
...
foreach (self::$loaders AS $loader) {
if (call_user_func($loader, $class) === true) {
return true;
}
}
self::$unloadable[] = $class;
return false;
}Given the vast differences in performance here I think preventing the double registration is the better solution. I'm not able to think of any conditions where that would be detrimental. |
|
@jrjohnson 👍 for checking performance! Just a tiny hint: making |
|
@alcaeus I made the change to |
|
@jrjohnson can you push your change here? I'd gladly check the performance issue then. |
|
Sure - changes are now pushed. I kept it as two commits for now, I will squash before merge. |
| 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; |
There was a problem hiding this comment.
Urgh, why do we even have our own autoloading garbage in here? :S
| @@ -122,18 +141,26 @@ static public function registerLoader($callable) | |||
| */ | |||
| static public function loadAnnotationClass($class) | |||
| { | |||
| if (isset(self::$loaded[$class])) { | |||
| AnnotationRegistry::loadAnnotationClass(self::class); | ||
| AnnotationRegistry::loadAnnotationClass('unloadableClass'); | ||
| AnnotationRegistry::registerLoader('class_exists'); | ||
| self::assertEmpty($this->getStaticField($this->class, 'loaded')); |
There was a problem hiding this comment.
This should be checked with an external loader, verifying that it will be called again. Please refrain from reading private state in a test
| AnnotationRegistry::loadAnnotationClass('unloadableClass'); | ||
| AnnotationRegistry::registerLoader('class_exists'); | ||
| self::assertEmpty($this->getStaticField($this->class, 'loaded')); | ||
| self::assertEmpty($this->getStaticField($this->class, 'unloadable')); |
|
@jrjohnson the code seems good so far, and a win-win overall for everyone. What could be happening in your test suite is that the annotation registry is being modified multiple times. Maybe you can profile it with xdebug+cachegrind and see where the method calls to the |
| @@ -110,6 +126,9 @@ static public function registerLoader($callable) | |||
| if (!is_callable($callable)) { | |||
| throw new \InvalidArgumentException("A callable is expected in AnnotationRegistry::registerLoader()."); | |||
| } | |||
| // Reset our static cache now that we have a new loader to work with | |||
| self::$loaded = array(); | |||
There was a problem hiding this comment.
Loaded classes do not need to be reset - there is nothing that we can reset anyway
…ust be using the internal autoloader
|
Moved to #137 @jrjohnson as for the performance issue caused by multiple |
Registering an expensive loader like
class_existstwice doubles thenumber of calls to the callable - this prevents that from happening.
This PR is a specific defensive response to
class_existsnow being added by both:https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Resources/config/annotations.xml#L17
and
https://github.com/symfony/phpunit-bridge/blob/master/bootstrap.php#L24
Which caused
class_existsto be called 133K more times in one example test.Edit: This is actually much worse than I originally thought. Because many of my functional tests boot kernels and the Registry is static by the middle of my test run there are 25+
class_existsregistered and everything grinds to a halt.