Drupal uses Composer for dependency management and autoloading.

Unlike many other projects, the vendor files and the generated autoloaders are part of the main Drupal repository. It mean that whenever there is a change in composer.json, the vendor files and the dumped autoloaders must be updated.

Unfortunately, it's not always done, especially for the autoloaders (composer dump-autoloader --optimize). It's not a problem as Drupal still works correctly but the performance is worse than it should be (as Composer is falling back to the regular autoloader when a file is not in the class map.)

So, this patch does several things:

  • Updates the class map;
  • Removes an old entry in composer.json;
  • Adds all core modules in the class map (to reduce the number of PSR-4 prefixes which slow down autoloading a lot);
  • Adds a unit test that will fail if someone forgot to update the class map when updating the composer.json file.

To sum up, the composer dump-autoloader --optimize must be run before submitting a Drupal patch if it modifies the composer.json file by adding or upgrading a dep or if it adds/removes classes in a core module.

The internal PSR-4 handling can be optimized even more but that will be part of another patch (the core modules should not be part of your PSR-4 prefix registration as they are now already managed by Composer.)

CommentFileSizeAuthor
#7 patch-classmap-fixed.patch542.91 KBfabpot
#4 patch-classmap-rebased.patch542.68 KBfabpot
patch-classmap-correct.patch537.89 KBfabpot
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fabpot’s picture

fabpot CreditAttribution: fabpot commented
Title: Huge performance improvement » Improve Composer performance
fabpot’s picture

fabpot CreditAttribution: fabpot commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, patch-classmap-correct.patch, failed testing.

fabpot’s picture

fabpot CreditAttribution: fabpot commented
FileSize
patch-classmap-rebased.patch542.68 KB
fabpot’s picture

fabpot CreditAttribution: fabpot commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: patch-classmap-rebased.patch, failed testing.

fabpot’s picture

fabpot CreditAttribution: fabpot commented
FileSize
patch-classmap-fixed.patch542.91 KB
fabpot’s picture

fabpot CreditAttribution: fabpot commented
Status: Needs work » Needs review
moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Priority: Normal » Critical
Issue tags: +Performance
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

Closed #2139601: Consider using composer's classmap feature for known clasess as duplicate. Xhprof stats over there

Crell’s picture

Crell CreditAttribution: Crell commented

Can you include a version of the patch that excludes mundane updates in the vendor libs themselves? Dreditor has a hard time with 500kb patches, and so do most reviewers. :-)

(Insert rant about how we're stupid for including the vendor libs themselves in our repo in the first place.)

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs review » Needs work 
-      "Drupal\\Driver\\": "drivers/lib/Drupal/Driver"
+      "Drupal\\": "core/modules"

This has the nice side effect of generating a map for all the core modules, but it is not actually correct. We don't have a PSR-4 namespace rooted in core/modules, what we have is several PSR-4 namespaces rooted in [module dir]/src.

fabpot’s picture

fabpot CreditAttribution: fabpot commented

I cannot exclude the changes in the autoload classes as this is the point of the new test: checking that the map is up to date.

fabpot’s picture

fabpot CreditAttribution: fabpot commented

Damien, you mean "semantically" wrong? Because it "works" as is. Would you prefer to list all module paths here instead of this shortcut?

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

@fabpot: yes, it is semantically wrong, I assume it only works because Composer introspects the namespace definition directly from inside the file while building the class map.

Given this definition:

"psr-4": {
  "Drupal": "core/modules"
}

A file at core/modules/block/src/Controller/BlockAddController.php should be in the Drupal\block\src\Controller namespace, not the correctly detected Drupal\block\Controller:

  'Drupal\\block\\Controller\\BlockAddController' => $baseDir . '/core/modules/block/src/Controller/BlockAddController.php',
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Does the classmap change mean that classes from all core modules can be autoloaded regardless of whether the modules are installed or not? If so, are we OK with that? Im aware that for optional dependencies of modules on other modules ModuleHandlerInterface::moduleExists() should be used instead of class_exists() directly. So in practice it shouldn't make a difference. It still feels wrong, however, in my opinion. (I might be missing something and that's actually not the case.)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs work » Closed (duplicate)

Sorry this is a duplicate of #1818628: Use Composer's optimized ClassLoader for Core/Component classes, see you over there. #16 and other drawbacks to this approach have already been discussed on that and related issues.

fabpot’s picture

fabpot CreditAttribution: fabpot commented

Well, #1818628 does not have the unit test I've added here to ensure that there is no regression over time. And it does not fix the error in composer.json.