Use ClassLoader instead of UniversalClassLoader

Using straight up ClassLoader makes complete sense here.

The APC ClassLoader is removed as it doesn't work with what we have (drupal_classloader_register() gives an exception). In replacement of it, once generate classmaps for all PSR-0 packages hits, we should switch to the Composer ClassLoader as there would be a 1:1 classmap given to us. This would make it extremely fast as we wouldn't even need to check the PSR-0 namespaces. This must follow after #1424924: Use Composer for updating Symfony components (without removing Symfony code from repo) hits though.

In the mean time, we're using Symfony's straight up ClassLoader without its APC counterpart.

Forgot to remove a line.

+++ b/core/includes/bootstrap.inc
@@ -2994,45 +2993,21 @@ function drupal_classloader() {
+      'Drupal\\Core' => DRUPAL_ROOT . '/core/lib',
+      'Drupal\\Component' => DRUPAL_ROOT . '/core/lib',

I think we decided on a coding standard of not including the \\ in single-quoted class name strings, didn't we?

Aside from that quibble, I'm fine here.

Typo.

I think we decided on a coding standard of not including the \\ in single-quoted class name strings, didn't we?

Updated

Let's post-pone this one until #1424924: Use Composer for updating Symfony components (without removing Symfony code from repo) gets in. Angie wanted to bring the needs review threshold down.

Should we do some benchmarks here?

Should we do some benchmarks here?

They'd be along the same lines as the benchmarks catch did in Explicitly register \Drupal\Component and \Drupal\Core:
before.png
after.png

This needs work as we might actually be able to make ApcClassLoader work for us here.

Patch looks pretty straightforward and I'm inclined to RTBC it, but it sounds like we want to see some benchmarks first, so tagging.

Related: #1807662: Built-in APCu support in core (PHP 5.5 only)

#1620114: Explicitly register \Drupal\Component and \Drupal\Core to increase class lookup performance first :-)

#1620114: Explicitly register \Drupal\Component and \Drupal\Core to increase class lookup performance landed, the drop is moving so fast :)

Adding a tag

Let's rerole the patch against recent changes in this part.

Oh right, annotations have to be converted as well to use the new namespaces

Looks good to me.

Unless we need benchmarks here (I doubt it?), this should be ready to fly.

I'm not sure whether this is just random fluctuation (running 100 requests and aggregated here).
This is not a normal drupal page you see here, one with pretty heavy class usage.

Note: this patch IS using the APCClassLoader part, see #1658720-1: Use ClassLoader instead of UniversalClassLoader, maybe robloach should review as well before getting this in.

1) Hm. So ClassLoader is minimally/slightly slower, and involves more function calls.

2) I looked at the ApcClassLoader docs and the changes in this patch seem to be correct.

+++ b/core/includes/bootstrap.incundefined
@@ -3111,50 +3111,29 @@ function drupal_classloader() {
+    if (variable_get('autoloader_mode', 'default') == 'apc') {
+      require_once DRUPAL_ROOT . '/core/vendor/symfony/class-loader/Symfony/Component/ClassLoader/ApcClassLoader.php';
+      $loader = new ApcClassLoader('drupal.' . $GLOBALS['drupal_hash_salt'], $loader);

Putting the benchmarks aside, it looks like we're instantiating a ClassLoader twice. Once using "ClassLoader", and then again later on if it's APC.

Yes, that's how ApcClassLoader is supposed to be used. Make sure to read its docs:
http://drupalcode.org/project/drupal.git/blob/HEAD:/core/vendor/symfony/...

Ah, thanks!

Twig doesn't use PSR-0 classes, so are we sure that we can use the non-universal class loader here?

Twig doesn't use PSR-0 classes, so are we sure that we can use the non-universal class loader here?

PSR-0 covers PEAR-like autoloading as it replaces any "_" in the class name with a directory separator for the class's path. You can see that on line 181:

    $classPath .= str_replace('_', DIRECTORY_SEPARATOR, $className) . '.php';

In Twig's composer.json, you can see it asks for the Composer's PSR-0 ClassLoader, which pretty much does the same thing. The benefit of using Symfony's straight up ClassLoader is that Symfony's ClassLoader component comes with the APC one. Composer's performance optimization comes in with composer dump-autoload --optimize, which caches a straight up classmap array, which would help even APC. But, that's a separate issue: #1818628: Use Composer's optimized ClassLoader for Core/Component classes.

More benchmarks:

=== core..core-1658720--core compared (508d81486ceb0..508d826a1e350):

ct  : 37,686|37,686|0|0.0%
wt  : 143,797|143,618|-179|-0.1%
cpu : 144,009|140,009|-4,000|-2.8%
mu  : 9,707,000|9,707,000|0|0.0%
pmu : 9,810,424|9,810,424|0|0.0%

=== core..core-1658720--19 compared (508d81486ceb0..508d833c057f5):

ct  : 37,686|37,830|144|0.4%
wt  : 143,797|144,021|224|0.2%
cpu : 144,009|144,010|1|0.0%
mu  : 9,707,000|9,705,784|-1,216|-0.0%
pmu : 9,810,424|9,808,944|-1,480|-0.0%

ct = number of function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

First benchmark is baseline, which shows some slight variance here of 179 micro secs.

144 function calls more, but the overhead is 224 micro secs plus, which is within variance, I think this is fine. The overhead are more calls to strpos (135, besides that CL uses file_exists instead of is_file, but that makes no real difference. file_exists is also called 5 times more in this benchmark.

I think this is good to go!

Tested with

$conf['autoloader_mode'] = 'apc';

Got:

PHP Fatal error:  Call to undefined method Symfony\Component\ClassLoader\ApcClassLoader::addPrefixes() in /var/www/core/includes/bootstrap.inc on line 3128
PHP Fatal error:  Class 'Symfony\Component\DependencyInjection\ContainerBuilder' not found in /var/www/core/includes/bootstrap.inc on line 2437

Please fix.

Moved over to #1834594: Update dependencies (Symfony and Twig) follow up fixes for Composer.

The classloader change should happen here. It doesn't make sense to do it in that issue, because as you can obviously see, it requires discussion, reviews, and benchmarks.

For the record: Performance-tweaking about our class loader right now is an epic waste of time. We can, by design, swap our class loader at any time without impacting functionality. We could do so post-freeze, post-code-freeze, hell we could do it in June or July or August and it would be fine. Right now the focus should be on easy, not micro-optimized. Do what makes development each, and stuff the rest. Revisit before release. Done.

Composer's class loader "just works" without much additional effort, and we're still adding libraries through it and updating others. Using that for now makes that process easier. Easy trumps optimized for the next 3 months at least.

#33: The only thing that needs work here is that the APC Class Loader is not working.

That is my only complaint of this patch.

I am not sure why the ApcClassLoader Decorator is not implementing addPrefixes, but the normal ClassLoader is, because I think we are doing this right.

Lets get a new patch in and unblock the update.

I am happy to test.

Right now the focus should be on easy, not micro-optimized. Do what makes development each, and stuff the rest. Revisit before release. Done. Composer's class loader "just works" without much additional effort, and we're still adding libraries through it and updating others. Using that for now makes that process easier. Easy trumps optimized for the next 3 months at least.

+1.... As many have said, Premature optimization is the root of all evil.... But we desperately need the Symfony and Composer updates in as soon as possible. I'm a fan of whatever will get those in sooner rather than later.

-    // Get all PSR-0 namespaces.
-    $namespaces = drupal_classloader()->getNamespaces();
-    foreach ($namespaces as $ns => $namespace_dirs) {
+    // Get all PSR-0 prefixes.
+    $prefixes = drupal_classloader()->getPrefixes();
+    foreach ($prefixes as $ns => $prefix_dirs) {

I don't think getPrefixes() is a public method on every PSR-0 classloader. If it's not, could we look at removing the dependency on whichever classloader we happen to be using from the plugin discovery?

If APC was selected to be used, but was not available, the above patch would fail. This one fixes that. Is that function_exists() check really needed? I'd assume if they're asking to use APC, then they know it's there.

I don't think getPrefixes() is a public method on every PSR-0 classloader. If it's not, could we look at removing the dependency on whichever classloader we happen to be using from the plugin discovery?

Correct... This is not a limitation of PSR-0, however. PSR-0 is simply an ideology, not an interface. Most systems wouldn't get a list of namespaces that the classloader has registered. You're right that we should remove that dependency. Let's do that here: #1836008: Remove drupal_classloader() use in Drupal\Core\AnnotatedClassDiscovery

Please disregard and see below.

===================

#37: Not necessary.

+++ b/core/includes/bootstrap.incundefined
@@ -3112,64 +3112,45 @@ function ip_address() {
+    if ($mode == 'apc' && function_exists('apc_store')) {
+      require_once DRUPAL_ROOT . '/core/vendor/symfony/class-loader/Symfony/Component/ClassLoader/ApcClassLoader.php';
+      $apc_loader = new ApcClassLoader('drupal.' . $GLOBALS['drupal_hash_salt'], $loader);
+      $apc_loader->register();

Unnecessary, because APCClassLoader does:

  if (!extension_loaded('apc')) {
            throw new \RuntimeException('Unable to use ApcClassLoader as APC is not enabled.');
        }

BUT the real problem is:

+++ b/core/includes/bootstrap.incundefined
@@ -3112,64 +3112,45 @@ function ip_address() {
-    $loader->registerNamespaces(array(
+    $loader->addPrefixes(array(
       'Drupal\Core' => DRUPAL_ROOT . '/core/lib',
...
-    $loader->register();
+    $loader->addPrefixes($namespaces);
   }
   return $loader;

This needs to happen, before the wrapping, because APCClassLoader does not implement addPrefixes method.

See: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Cla...

+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined
@@ -47,14 +47,14 @@ public function getDefinitions() {
+    // Get all PSR-0 prefixes.
+    $prefixes = drupal_classloader()->getPrefixes();
+    foreach ($prefixes as $ns => $prefix_dirs) {

And this method also does not exist.

The APCClassLoader is pretty basic.

Can we add our own APCClassLoader that acts like a real decorator / proxy and implements the same functions as the ClassLoader?

============================================

Ah, I got it now :-).

We are just registering the APCClassLoader, but using the normal class loader still for getPrefixes, etc.

Makes sense :-).

Maybe use try / catch to make use of Exceptions?

  $registered = FALSE;
  if ($mode == 'apc') {
    try {
      $apc_classloader->register();
      $registered = TRUE;
    } catch (Exception $e) {
    }
  }
  if (!$registered) {
    $loader->register();
  }

Test failures seem to be irrelevant.

Regardless of that, slightly revamped this patch, and also filed https://github.com/symfony/symfony/pull/5950

The good news: @fabpot merged https://github.com/symfony/symfony/pull/5950 already. :)

The bad news: This patch causes a 300% slowdown in performance. A single web test that normally takes ~20 secs takes ~60 seconds.

I already checked whether the most current ClassLoader in Symfony contains any noteworthy changes compared to the current in core, but there are none.

The only thing that stands out in the callgrind is an exceptional amount of time being spent in annotation parsing. See attached.

It's possible that this is caused by #1836008: Remove drupal_classloader() use in Drupal\Core\AnnotatedClassDiscovery and we might need to resolve that first.

Since we blindly use any namespace registered to the classloader, I have a feeling we're scanning everything in vendor/ for plugins. Haven't verified this but it came up when EclipseGc asked me about this issue on irc yesterday.

#39: drupal8.classloader.39.patch queued for re-testing.

#45: 1658720.patch queued for re-testing.

Anyone know why this fails? We might need the Bootstrap's Bootstrap fix...

[Assetic's classes fail to be loaded] because our classloader decides it's a PEAR-style library rather than being properly namespaced & PSR0 […] in the meantime, i've rolled an updated patch including the two-character fix that makes it work

From #1762204-15: Introduce Assetic compatibility layer for core's internal handling of assets.

This patch needs an update. Sun's ClassLoader decorator updates have made it into Symfony now too. Hopefully Symfony has been updated in Core by now as well.

Rerolled against all the recent changes. I tested a drupal installation and it worked pretty fine.

The original patch contained some "random" changes in some of the composer files, so it seems to be that we should update to recent versions.

For everyone following at home:

Fatal error: Call to undefined method Symfony\Component\ClassLoader\ClassLoader::registerPrefixes()
 in /core/lib/Drupal/Core/DrupalKernel.php on line 313
FATAL Drupal\system\Tests\DrupalKernel\DrupalKernelTest: test runner returned a non-zero error code (255).

Fatal error: Maximum execution time of 500 seconds exceeded
 in /core/vendor/doctrine/common/lib/Doctrine/Common/Annotations/TokenParser.php on line 53
Segmentation fault
FATAL Drupal\comment\Tests\CommentPreviewTest: test runner returned a non-zero error code (139).

Fatal error: Maximum execution time of 500 seconds exceeded
 in /core/vendor/doctrine/common/lib/Doctrine/Common/Reflection/StaticReflectionParser.php on line 143
FATAL Drupal\text\Tests\TextSummaryTest: test runner returned a non-zero error code (255).

Fatal error: Maximum execution time of 500 seconds exceeded
 in /core/vendor/doctrine/common/lib/Doctrine/Common/Annotations/TokenParser.php on line 69
FATAL Drupal\text\Tests\TextTranslationTest: test runner returned a non-zero error code (255).

Fatal error: Maximum execution time of 500 seconds exceeded
 in /core/lib/Drupal/Component/Plugin/Discovery/AnnotatedClassDiscovery.php on line 88
FATAL Drupal\filter\Tests\FilterAdminTest: test runner returned a non-zero error code (255).

Fatal error: Maximum execution time of 500 seconds exceeded
 in /core/vendor/doctrine/common/lib/Doctrine/Common/Reflection/StaticReflectionParser.php on line 147
FATAL Drupal\system\Tests\Module\ModuleApiTest: test runner returned a non-zero error code (255).

Fatal error: Maximum execution time of 500 seconds exceeded
 in /core/vendor/doctrine/common/lib/Doctrine/Common/Reflection/StaticReflectionParser.php on line 165
FATAL Drupal\rdf\Tests\RdfaMarkupTest: test runner returned a non-zero error code (255).

Fatal error: Maximum execution time of 500 seconds exceeded
 in /core/vendor/doctrine/common/lib/Doctrine/Common/Reflection/StaticReflectionParser.php on line 165
FATAL Drupal\search\Tests\SearchCommentTest: test runner returned a non-zero error code (255).

Fatal error: Cannot redeclare class Drupal\Core\Plugin\Validation\Constraint\PrimitiveTypeConstraint
 in /core/lib/Drupal/Core/Plugin/Validation/Constraint/PrimitiveTypeConstraint.php on line 24
FATAL Drupal\system\Tests\TypedData\TypedDataTest: test runner returned a non-zero error code (255).

1) "Segmentation fault". *ugh*

2) Still a massive slow-down in Annotation parser code/infrastructure. *doubleugh*

3) Fatals on registerPrefixes() and duplicate PrimitiveTypeConstraint class names are most probably real. *tripleugh* ;)

addPrefixes().

Uploading the correct patch would help.

+++ b/core/vendor/composer/autoload_namespaces.php
@@ -25,6 +25,9 @@
     'Guzzle\\Http' => $vendorDir . '/guzzle/http/',
     'Guzzle\\Common' => $vendorDir . '/guzzle/common/',
     'EasyRdf_' => $vendorDir . '/easyrdf/easyrdf/lib/',
+    'Drupal\\Driver\\' => $baseDir . '/lib',
+    'Drupal\\Core\\' => $baseDir . '/lib',
+    'Drupal\\Component\\' => $baseDir . '/lib',
     'Doctrine\\Common' => $vendorDir . '/doctrine/common/lib/',
     'Assetic' => $vendorDir . '/kriswallsmith/assetic/src/',

Can you clarify whether this change is auto-generated?

Whatever the technical lookup facility/method is, the vast majority of class file name lookups will be considerably faster when checking for Drupal\Component + Drupal\Core first.

Everything in core/vendor/composer is auto-generated by Composer.

Instead of

+    "psr-0": {
+      "Drupal\\Core\\": "lib",
+      "Drupal\\Component\\": "lib",
+      "Drupal\\Driver\\": "lib"
+    }

It should be:

+    "psr-0": {
+      "Drupal\\Core": "lib",
+      "Drupal\\Component": "lib",
+      "Drupal\\Driver": "lib"
+    }

Let's see whether this last addition should fix it.

Thanks chx!

Good catch, chx.

#72: 1658720_70.patch queued for re-testing.

Just another rerole, though we still struggle with #1658720-41: Use ClassLoader instead of UniversalClassLoader

I try hard to find the real reason, but I didn't succeeded yet.

Just me. (Kicking it off the bot so we can test the 5.3.10 upgrade. Has been re-queued on a different bot.)

There are still some follow ups from #1836008: Remove drupal_classloader() use in Drupal\Core\AnnotatedClassDiscovery, so I didn't touch AnnotatedClassDiscovery, but here's an updated patch.

Splitting the tests into their own respective test functions fixes the issue.

Just fixing two small nitpicks. This looks really good as it is.

Thanks!

Looks great to me. Thanks all!

+++ b/core/vendor/composer/autoload_namespaces.phpundefined
@@ -6,6 +6,9 @@
+    'Drupal\\Driver' => $baseDir . '/lib/',

I guess this should be 'drivers' instead of $baseDir (=== "core")? At least it looks not equivalent to what was changed.

Good catch, Fabian. In order to match what was there previously, it should be "../drivers/lib/".

There's a follow up at #1805316: Move composer.json to the project root which would allow us to avoid transversing up a directory.

Back to RTBC

This patch needs an update... #87: 1658720.patch queued for re-testing.

Confirmed working on APC.

This changes look fine.

Updated summary

Seems like catch has been all over this one, so assigning to him.

#93: 1658720.patch queued for re-testing.

#93: 1658720.patch queued for re-testing.

Guaranteed to need reroll after #1805316: Move composer.json to the project root

Also, it doesn't look like we ever got fresh benchmarks after we figured out this degraded performance by 300%. Let's do that before re-assigning to catch.

Comment number 100 and a reroll :)

+++ b/composer.jsonundefined
@@ -21,6 +21,13 @@
     "phpunit/phpunit": "3.7.15"
   },
+  "autoload": {
+    "psr-0": {
+      "Drupal\\Core": "lib/",
+      "Drupal\\Component": "lib/",
+      "Drupal\\Driver": "../drivers/lib/"
+    }
+  },
   "minimum-stability": "dev",

Now that composer.json is in the root, it should probably be:

+  "autoload": {
+    "psr-0": {
+      "Drupal\\Core": "core/lib/",
+      "Drupal\\Component": "core/lib/",
+      "Drupal\\Driver": "drivers/lib/"
+    }

I'll be able to re-roll tomorrow if one isn't up by then! :-)

Oh right, I'm wondering whether we can and should the composer file.

Re-roll with a couple other new instances.

These changes look good.

Fresh benchmarks?

#103: 1658720.patch queued for re-testing.

Reports from xdebug in percentage cost...

UniversalClassLoader (current)

ClassLoader (#103)

As I've said before, although the benchmarks do show improvement, the primary benefit of this is not performance. The primary reason for this is that UniversalClassLoader requires you to register This\Type\Of\Class (how we namespace our classes) and This_Type_Of_Class (PEAR-like) differently (PEAR-like is used in Twig). ClassLoader directly supports PSR-0, which covers both methodologies.

This issue isn't about performance, it's about class loading sanity.

So back to RTBC.

Yeah, I understand this issue isn't about performance, but earlier patches showed a 300% slowdown in performance, which is totally unacceptable.

Re-assigning to catch for sign-off, thanks!

This looks much better now. My original objection here was to ditching the APC class loader, I'm not yet convinced that a generated classmap is going to be better than APC for Drupal sites, since we tend to have a very high proportion of un-used classes which I doubt most Symfony projects do, at least not without some customization. This doesn't do that any more though and the diff looks great so committed/pushed to 8.x.

It'd be good if changing the classloader didn't require such a large diff, does there need to be a PSR for the classloaders themselves :(

Does there need to be a PSR for the classloaders themselves

Not sure if this would be viable since there are so many different ways of loading classes. Definitely worth considering, however. Psr\Log has proved helpful, Psr\Loader\LoaderInterface could be deemed useful as well.