Use Composer's defined namespaces to ease maintenance

+++ 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.

OK, I'm happy then. :-)

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

How about just this?

Just a comment tweak.

Even more straightforward.

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

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

Meant to do this before...

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.

Rerolled for #1683046-24: Add the Doctrine Common PHP library. Leaving as RTBC.

Typo. Thanks!

Committed/pushed to 8.x, thanks all.