Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent the same loader from being registered twice #135

Closed

Conversation

jrjohnson
Copy link
Sponsor Contributor

@jrjohnson jrjohnson commented Jul 6, 2017

Registering an expensive loader like class_exists twice doubles the
number of calls to the callable - this prevents that from happening.

This PR is a specific defensive response to class_exists now 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_exists to 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_exists registered and everything grinds to a halt.

Registering an expensive loader like `class_exists` twice doubles the
number of calls to the callable - this prevents that from happening.
@Ocramius
Copy link
Member

@jrjohnson shouldn't the loader simply stop dispatching further autoloaders if class_exists($annotationClass, false) reports true?

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.

@jrjohnson
Copy link
Sponsor Contributor Author

The problem isn't when there is a hit, it is when there is a miss. Something like @package or @returns might exist thousands of times in a code base but never be hit by an autoloader.

How would you change the order of autoloaders by registering them twice? If you have [class_exists, my_autoloader] and you add them again in the reverse order you would end up with [class_exists, my_autoloader, my_autoloader, class_exists]. The only way you could change that would be to call reset() which wouldn't be effected by this change.

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.

@Ocramius
Copy link
Member

@jrjohnson here's what we can do, in my opinion (and would even be faster than preventing double autoloader registration):

  • register a static property storing all names of classes that failed annotation autoloading, skip those directly if a lookup fails
  • clear that list every time an autoloader is added to the stack

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.

@jrjohnson
Copy link
Sponsor Contributor Author

@Ocramius thanks, I will get started on that. Is there any reason not to cache the successes as well while I'm at it?

@jrjohnson
Copy link
Sponsor Contributor Author

@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:
You can see that class_exists still get's called a lot even when misses are cached:
https://blackfire.io/profiles/compare/a08223eb-a0da-49a9-ac4b-eff8510c016a/graph

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.

@alcaeus
Copy link
Member

alcaeus commented Jul 14, 2017

@jrjohnson 👍 for checking performance! Just a tiny hint: making $unloadable an assoc array with the class name as key and using isset should improve performance considerably (in my tests by a factor of 10): https://gist.github.com/alcaeus/536156663fac96744eba77b3e133e50a

@jrjohnson
Copy link
Sponsor Contributor Author

@alcaeus I made the change to isset and it doesn't help appreciably in this case. I think $unloadable just gets cleared way too often in tests to be a useful cache.

@Ocramius
Copy link
Member

@jrjohnson can you push your change here? I'd gladly check the performance issue then.

@jrjohnson
Copy link
Sponsor Contributor Author

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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])) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looking good 👍

AnnotationRegistry::loadAnnotationClass(self::class);
AnnotationRegistry::loadAnnotationClass('unloadableClass');
AnnotationRegistry::registerLoader('class_exists');
self::assertEmpty($this->getStaticField($this->class, 'loaded'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

@Ocramius
Copy link
Member

@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 AnnotationRegistry come from?

@@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loaded classes do not need to be reset - there is nothing that we can reset anyway

Ocramius added a commit that referenced this pull request Jul 22, 2017
Ocramius added a commit that referenced this pull request Jul 22, 2017
Ocramius added a commit that referenced this pull request Jul 22, 2017
Ocramius added a commit that referenced this pull request Jul 22, 2017
@Ocramius
Copy link
Member

Moved to #137

@jrjohnson as for the performance issue caused by multiple reset() or registerLoader() calls, that cannot be fixed without introducing BC breaks :(

@Ocramius Ocramius closed this Jul 22, 2017
@Ocramius Ocramius added this to the v1.5.0 milestone Jul 22, 2017
@Ocramius Ocramius self-assigned this Jul 22, 2017
Ocramius added a commit that referenced this pull request Jul 22, 2017
Ocramius added a commit that referenced this pull request Jul 22, 2017
@jrjohnson jrjohnson deleted the double-loader-registration branch July 22, 2017 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants