Composer provides us with a ClassLoader that will automatically register the Symfony/Twig and Drupal PSR-0 namespaces for us. It does this via a autoload.php. We currently have some hard-coded namespace registration which doesn't need to be with autoload.php around.

Benchmarks

The main reason for switching over to the Composer ClassLoader is not the performance gain, but I thought it would be a good idea to provide the numbers.

UniversalClassLoader-xhprof.png
This is using our current solutionw ith Symfony's UniversalClassLoader and drupal_classloader() (13.1% CPU at 32ms)
ComposerClassLoader-xhprof
This is in the proposed patch, which simply switches to the Composer ClassLoader (8.8% CPU at 20ms)
ComposerClassLoader-ClassMap-xhprof
There is a pull request in Composer to generate a classmap of all the PSR-0 namespaces. This gives a 1:1 class to path reference which really speeds things up. It still works with development too, because if the class isn't found, it'll still use the namespaces. Out of curiousity, I thought I'd do a benchmark on it... (1.9% CPU at 4ms)

For this issue, we simply switch to the Composer ClassLoader. Once the ClassMap generator is brought into Composer core, we'll automatically gain additional performance without even really doing anything.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RobLoach’s picture

Issue summary: View changes

links

RobLoach’s picture

Issue summary: View changes

f

RobLoach’s picture

Issue summary: View changes

f

RobLoach’s picture

Issue summary: View changes

Updated issue summary.

RobLoach’s picture

Tagging.

RobLoach’s picture

Issue summary: View changes

f

Crell’s picture

+++ b/core/vendor/composer/ClassLoader.php
@@ -102,7 +102,7 @@ class ClassLoader
     /**
      * Turns on searching the include path for class files.
      *
-     * @param Boolean $useIncludePath
+     * @param bool $useIncludePath
      */

Why are there doc changes inside the generated ClassLoader.php file? Are we doing that, or did Composer change something in its generated code?

Other than that, this looks quite sane to me.

RobLoach’s picture

That was a coding standards fix from them.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

OK, I'm happy then. :-)

webchick’s picture

Assigned: RobLoach » catch

Over at #1424924-121: Use Composer for updating Symfony components (without removing Symfony code from repo), catch was pretty adamant about this getting proper review before going into core. I'm assigning to him to take a look at.

catch’s picture

I'm not sure a classmap is going to be a great idea for Drupal sites, which could have potentially thousands of classes in the filesystem - there's likely to be a tipping point between individual lookup of say 200 classes vs. loading a very large classmap. However it's interesting that's might be added to composer and it's something we could look at more in #1241190: Possible approaches to bootstrap/front loading of classes.

Some questions:

How many classes were in the classmap when you profiled this?

How long does it take to generate the classmap, and what triggers this? (is it done on demand or do you just trigger it from cli then forget?)

Is the cost of loading the classmap on each request included in your profiling (i.e. in loadclass()), or does that happen elsehere? If elsewhere how long did it take?

Do you know why there are different number of classes loaded for each request when profiling? Each xhprof screenshot shows a different number of function calls. This is usually a sign of a cache miss for something, which can throw off numbers for everything else.

RobLoach’s picture

Those questions might be better answered over at:
https://github.com/composer/composer/pull/811

In the mean time, it would be great to get this in so that namespaces are handled by Composer (ComposerClassLoader-xhprof). This allows Symfony and Twig to register their own namespaces. If a new component is added, it'll be handled by autoload.php, otherwise, that component's namespaces are missing unless we add it manually to drupal_classloader().

catch’s picture

This question should definitely be answered before this gets committed unless someone does another round of profiling to confirm:

Do you know why there are different number of classes loaded for each request when profiling? Each xhprof screenshot shows a different number of function calls. This is usually a sign of a cache miss for something, which can throw off numbers for everything else.

I'll try to post the others over at the pull request though.

catch’s picture

Status: Reviewed & tested by the community » Needs review

Back to CNR for the profiling questions, if I can find some time I'll try to go over it myself but great if someone can beat me to it.

RobLoach’s picture

This is much faster than what we currently have, and it fixes problems we run into with namespaces not being registered when we add new components, like some Doctrine (like in the plugins system via #1683046: Add the Doctrine Common PHP library). I'm pushing back to RTBC.

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

classloader.patch queued for re-testing.

msonnabaum’s picture

From what I can tell, composer doesn't yet support an APC loader, so this seems like a big performance regression to me.

The classmap is great, but it appears to be unfinished still. We also shouldn't rely on this, because as catch points out, there is a memory cost to loading that on every request.

That said, I think the decision to use this should be based more on our needs than performance. Worst case we push Composer to support an APC loader or we just use the static namespaces that composer writes out and register them with the existing autoloaders.

catch’s picture

Status: Reviewed & tested by the community » Needs work

OK let's open a feature request for Composer to at least support an APC class loader before this goes in then and/or a follow-up issue to track that.

Crell’s picture

Status: Needs work » Reviewed & tested by the community

Let me see if I understand this...

We want to switch from our current class loader setup to one that is faster, and easier for us to keep up to date, and we're marking it needs-work because it could maybe be even faster than that? Because it's not faster *enough* to justify... making our lives easier at the same time.

I am confused.

catch’s picture

Status: Reviewed & tested by the community » Needs work

No. We're moving from a classloader that ships with several implementations, including an APC classloader that's recommended for production, to one that has no APC class loader, and an in-progress pull request for a classmap which may or may not make sense on production Drupal sites.

We already have an issue to do more analysis of the Symfony classloader since it is showing up quite high in profiling results already, despite Drupal 8 having likely a fraction of the classes loaded on each request that it eventually will.

However with Symfony there is a settings.php option to switch to the APC loader for which there's a reasonable expectaction it'll be 'OK'. Since we're dropping that option altogether, there needs to be a plan to fix it. No such plan exists at the moment, so 'needs work'.

Also, there is only one set of profiling results saying that the Composer class loader is 'faster', and they have unanswered questions since July 11th, so I'm also confused why people are repeatedly bumping this back to RTBC without answering any of my questions or apparently even reading the issue.

catch’s picture

And weighing this up I'm still not sure this a good trade-off. Registering namespaces automatically for us is only going to save about 10 minutes work once per month when we add a new library to core, is it really worth dropping the APC class loader for that then having to do potentially a bunch more work to get it reimplemented in Composer? There's absolutely no other justification given for switching at all in this issue.

Crell’s picture

Actually a class map is known to be faster than a FS lookup. Plenty of benchmarks already say that:

http://mwop.net/blog/245-Autoloading-Benchmarks.html
http://athos.blogs.balabit.com/2011/03/php-autoload-performance/

So at best, we wait for the classmap version of Composer to land, then switch to it.

RobLoach’s picture

In the mean time, we could switch to using core/vendor/autoload_namespaces.php to retrieve the list of required namespaces. I'd still much rather just use the Composer classloader as it's still faster than what we currently have.

webchick’s picture

I'd still much rather just use the Composer classloader as it's still faster than what we currently have.

You keep saying this, but yet you have not addressed the questions from #8, which casts that assertion into doubt.

catch’s picture

Actually a class map is known to be faster than a FS lookup.

That's not the choice. The choice is between an FS lookup, a classmap and an APC cache of FS lookups, assuming we leave ourselves that choice.

I've also pointed out that a generated classmap based on all classes in a Drupal install could be massive. Note this line from the first benchmark you posted:

Timing was performed only over the loop.

i.e. that benchmark did not take into account the cost of initializing the classmap. The second link you posted doesn't even mention this detail either way so we have no idea what they were actually testing at all.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
3.33 KB

This eases maintenance, but still doesn't quite get us the speeds achieved in the original benchmarks, or the links Crell pointed to. It does ease maintenance though, as we won't have to manually add the namespaces when new projects come in.

You keep saying this, but yet you have not addressed the questions from #8, which casts that assertion into doubt.

I haven't had the time to run through the benchmarks again. Any help with that would be great... Been pretty busy lately.

msonnabaum’s picture

I think this is a great compromise. Auto-generating the namespaces is the main win here, we shouldnt be arguing to use composer's autoloader for performance reasons, because as catch and I have pointed out, that is not a compelling argument.

I do think we should eventually switch to using composer's autoloader, but losing APC is kinda huge. We should revisit switching loaders once that's an option, but for now getting just the namespaces is a win.

RobLoach’s picture

Title: Switch to the Composer ClassLoader » Use Composer's defined namespaces to ease maintenance
effulgentsia’s picture

FileSize
2.34 KB

How about just this?

effulgentsia’s picture

FileSize
2.33 KB

Just a comment tweak.

effulgentsia’s picture

FileSize
2.29 KB

Even more straightforward.

Crell’s picture

#26 is at least an improvement over the status quo. Let's do.

msonnabaum’s picture

Haven't tested it, but I totally support the approach in #26.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Meant to do this before...

catch’s picture

Yes latest patch looks great.

If someone actually sits down and does the work to get Composer's autoloader to the same level of maturity as Symfony's and/or does enough performance analysis of what's there to show there's no regression then I have no problem switching, but neither of those have happened yet. When they do we can revisit.

effulgentsia’s picture

FileSize
2.37 KB
RobLoach’s picture

+++ b/core/includes/bootstrap.incundefined
@@ -3038,23 +3038,23 @@ function drupal_classloader() {
+      }
+    };
+    $loader->registerPrefixes($prefixes);
+    $loader->registerNamespaces($namespaces);
+
     // Register the Drupal namespace for classes in core as a fallback.
     // This allows to register additional namespaces within the Drupal namespace
     // (e.g., for modules) and avoids an additional file_exists() on the Drupal

Any reason for the ; after the foreach()? Other than that, RTBC!

effulgentsia’s picture

FileSize
2.36 KB

Typo. Thanks!

RobLoach’s picture

#33: namespaces-33.patch queued for re-testing.

catch’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Performance

Committed/pushed to 8.x, thanks all.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.