Do not unnecessarily load replaced packages into the pool

[driskell]

I'm seeing slow performance with composer and Drupal. I thought it may be related to the Pool Optimiser PR not having much impact on Drupal installations (see https://github.com/composer/composer/pull/9261#issuecomment-703171395) but it might not be. To be specific, this is when performing updates. It heavily impacts dependabot.

To reproduce. Take the composer.json from https://github.com/drupal/recommended-project/blob/9.2.x/composer.json Then install composer install --profile Then install a module composer require drupal/media_entity_twitter:2.5.0 --profile Modify composer.json for media_entity_twitter to ^2.0 Then update that module composer update -W drupal/media_entity_twitter --profile (goes to 2.6.0 currently.)

The timings I get are:

$ composer --version
Composer version 2.0.7 2020-11-13 17:31:06
$ composer install --profile
[31.7MiB/31.07s] Memory usage: 31.66MiB (peak: 228.37MiB), time: 31.07s
$ composer require drupal/media_entity_twitter:2.5.0 --profile
[19.1MiB/3.14s] Memory usage: 19.07MiB (peak: 20.42MiB), time: 3.14s
$ nano composer.json
$ composer update -W drupal/media_entity_twitter --profile
[46.9MiB/25.46s] Memory usage: 46.86MiB (peak: 203.91MiB), time: 25.46s

As you can see the update takes a long time. When using a composer version that outputs the network requests, you can see many many 404 like these:

[23.5MiB/4.08s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-bridge.json
[23.5MiB/4.19s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-front-matter.json
[23.5MiB/4.34s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-diff.json
[23.5MiB/4.35s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-php-storage.json
[23.6MiB/4.43s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-discovery.json
[23.6MiB/4.52s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-file-cache.json
[23.6MiB/4.58s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-file-security.json
[23.6MiB/4.62s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-filesystem.json
[23.6MiB/4.83s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-http-foundation.json
[23.6MiB/4.87s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-graph.json
[23.6MiB/4.95s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-gettext.json
[23.6MiB/5.00s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-proxy-builder.json
[23.7MiB/5.05s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-render.json
[23.7MiB/5.06s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-uuid.json
[23.7MiB/5.09s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-plugin.json
[23.7MiB/5.29s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-utility.json
[23.7MiB/5.29s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/datetime.json
[23.7MiB/5.35s] [404] https://packages.drupal.org/files/packages/8/p2/drupal/core-transliteration.json

This goes on for a while.

Essentially the drupal/core-recommended depends on drupal/core package which has a large replaces list. This list contains the name of every core module. Whether or not it should is left for discussion elsewhere (there are reasons to do so.) This list of replaced packages for the most part do not exist - for example drupal/taxonomy is, and has always been, a core module (I think). When you run an update it sees that media_entity_twitter depends on drupal/core and thus unlocks it as a non-root dependency and marks it for load, but it then proceeds to load all the replacements - even though no other package ever requires them.

So it seems somewhat unnecessary to be loading the replaced packages. If there is indeed a require to one of them somewhere, it would make sense. Maybe I'm missing something.

With this PR the 404s disappear and the timings for the update becomes as expected:

$ composer update -W drupal/media_entity_twitter --profile
[29.7MiB/5.84s] Memory usage: 29.66MiB (peak: 250.26MiB), time: 5.84s

Savings of 20 seconds.

WDYT?

CC @Toflar after discussion in #9261 (Thanks!)

[driskell]

Hi @naderman @Seldaek Do you have any other questions about the change? Is it worth rebasing time check any new tests? Would be great to work out what might be able to add confidence to this as I appreciate it’s quite deep in the critical areas.

[stof]

replaces and provides should indeed not mark things for loading if they get unlocked IMO, as in a normal pool without any locking done, those don't trigger loading of metadata AFAIK.

[naderman]

@stof see https://github.com/composer/composer/pull/9619#discussion_r760237931 ?

[driskell]

@stof @naderman Regarding comment https://github.com/composer/composer/pull/9619#discussion_r760237931

I initially investigated this and found it was related to a different area of code: https://github.com/composer/composer/pull/9619#discussion_r776339911

As per comment https://github.com/composer/composer/pull/9619#discussion_r777023213 I raised PR #10410 with that test and also what I think is the fix for the test.

So I'm unsure if there's anything remaining outstanding except a closer scrutiny on the change in this PR, which as per the last comment above could be increased in scope to delete the entire section of code below without failing a single test. So I think the remaining questions are: Is this code that slows down composer needed? Is there a test that will fail without it?

https://github.com/composer/composer/blob/24ce1eddbdb4658e1d106a5d982951342f86f4d3/src/Composer/DependencyResolver/PoolBuilder.php#L468-L489

[jeffwidman]

👋 Hi from the Dependabot team.

I was digging into a few reports we've received of timeouts during composer updates, particularly Drupal and stumbled across this PR.

Given the last response from @driskell above seems to indicate this PR is pretty much ready, what else is needed to move this forward?

Is it just lack of maintainer time? (which as a fellow maintainer of several OSS projects, I realize is always in short supply!)

[naderman]

Yeah that's all it is, I've had these on my list for a while and am really sorry I still haven't gotten to it ...

[markdorison]

Hi all! We are still battling with the Dependabot timeouts on Drupal projects that we hope this may resolve. As Jeff mentioned, I completely understand your time is in short supply but I wanted to add a friendly bump to this. ❤️

[Toflar]

I did some in-depth analysis of this PR (finally - sorry @driskell) and I'm fairly confident that the approach is actually correct! There's no point in marking the replace links as required for download (markPackageNameForLoading()). Basically, a replace target is optional until some other package requires it. Because if not, then there's no point in loading something that is replaced anyway. So we can really just remove this one line 👍 There are only two tests that need adjustments (and actually prove that there are useless packages in the pool). So once this PR is rebased, it should look like this:

Index: src/Composer/DependencyResolver/PoolBuilder.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/Composer/DependencyResolver/PoolBuilder.php b/src/Composer/DependencyResolver/PoolBuilder.php
--- a/src/Composer/DependencyResolver/PoolBuilder.php	(revision 11879ea737978fabb8127616e703e571ff71b184)
+++ b/src/Composer/DependencyResolver/PoolBuilder.php	(date 1683036654517)
@@ -483,7 +483,6 @@
 
                     if ($request->getUpdateAllowTransitiveRootDependencies() || !$skippedRootRequires) {
                         $this->unlockPackage($request, $repositories, $replace);
-                        $this->markPackageNameForLoading($request, $replace, $link->getConstraint());
                     } else {
                         foreach ($skippedRootRequires as $rootRequire) {
                             if (!isset($this->updateAllowWarned[$rootRequire])) {
Index: tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/multi-repo-replace-partial-update-all.test
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/multi-repo-replace-partial-update-all.test b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/multi-repo-replace-partial-update-all.test
--- a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/multi-repo-replace-partial-update-all.test	(revision 11879ea737978fabb8127616e703e571ff71b184)
+++ b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/multi-repo-replace-partial-update-all.test	(date 1683024149715)
@@ -101,7 +101,6 @@
     "indirect/replacer-1.0.0.0",
     "replacer/package-1.2.0.0",
     "replacer/package-1.0.0.0",
-    "base/package-1.0.0.0",
     "shared/dep-1.0.0.0",
     "shared/dep-1.2.0.0"
 ]
@@ -112,6 +111,5 @@
     "indirect/replacer-1.0.0.0",
     "replacer/package-1.2.0.0",
     "replacer/package-1.0.0.0",
-    "base/package-1.0.0.0",
     "shared/dep-1.2.0.0"
 ]
Index: tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixing-with-replacers.test
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixing-with-replacers.test b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixing-with-replacers.test
--- a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixing-with-replacers.test	(revision 11879ea737978fabb8127616e703e571ff71b184)
+++ b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixing-with-replacers.test	(date 1683024343988)
@@ -46,9 +46,7 @@
     "root/req1-1.0.0.0",
     "root/req1-1.1.0.0",
     "replacer/pkg-1.0.0.0",
-    "replacer/pkg-1.1.0.0",
-    "replaced/pkg-1.2.3.0",
-    "replaced/pkg-1.2.4.0"
+    "replacer/pkg-1.1.0.0"
 ]
 
 --EXPECT-OPTIMIZED--
@@ -56,6 +54,5 @@
     "root/req3-1.0.0.0 (locked)",
     "dep/dep-2.3.5.0 (locked)",
     "root/req1-1.1.0.0",
-    "replacer/pkg-1.1.0.0",
-    "replaced/pkg-1.2.4.0"
+    "replacer/pkg-1.1.0.0"
 ]

[stof]

@Toflar how does this analysis interact with https://github.com/composer/composer/pull/10410 ?

[Toflar]

@stof actually, your comment made me think about "why do we need markPackageNameForLoading() in https://github.com/composer/composer/pull/10410 but here we want to get rid of it for replace packages?". So I've incorporated both PRs in another approach in https://github.com/composer/composer/pull/11449.

I hope this makes it much more readable because all it is is basically not calling markPackageNameForLoading() for replace definitions but only load them if any other package requires one of them 😊 Should also be easier to understand from the diff - I hope.

[naderman]

@driskell can you confirm this is fixed by the merge of https://github.com/composer/composer/pull/11449 now?

[naderman]

Same question for @markdorison

[markdorison]

I can test this once the latest changes from #11449 make it into Dependabot! @jeffwidman may be able to help with that or at least let us know when it has been included. 🥳

[jeffwidman]

Historically we've only used released composer versions to keep life simple, they get picked up by :dependabot: : https://github.com/dependabot/dependabot-core/pulls?q=is%3Apr+author%3Aapp%2Fdependabot+%22composer%2Fcomposer%22+

If you want to test ahead of time, you could theoretically do it locally, but my guess is it'd be painful to wire that in since our whole build wiring assumes a released version, eg https://github.com/dependabot/dependabot-core/blob/36c7ccb07121ae9d574b264c8bc4b27cb4b65bf3/composer/Dockerfile#L3

I assume #11449 will land in composer 2.6 so once that drops we can pick it up/deploy it within a few hours.

[driskell]

I can confirm this is fixed by the changes in main! No more unexpected 404s.

Profile results of the OP from running on 2.5.5 (using Drupal fixed to 9.2.* as that's a branch with replaces still in place):

[20.4MiB/7.64s] Memory usage: 20.36MiB (peak: 79.86MiB), time: 7.64s
[20.4MiB/8.06s] Memory usage: 20.36MiB (peak: 79.86MiB), time: 8.06s
[20.4MiB/7.77s] Memory usage: 20.36MiB (peak: 79.86MiB), time: 7.77s

Using commit 595559f68d8c01715a2be33a54247898f9fbc34e on main containing the changes:

[20.3MiB/3.99s] Memory usage: 20.28MiB (peak: 75.13MiB), time: 3.99s
[20.3MiB/3.77s] Memory usage: 20.28MiB (peak: 75.13MiB), time: 3.77s
[20.3MiB/3.69s] Memory usage: 20.28MiB (peak: 75.13MiB), time: 3.69s

And when modifying the CurlDownloader to log requests, all the unexpected 404s are gone.

I'll post in the Dependabot ticket some timings for Dependabot too. I tested 9.5.* too which no longer has replaces and it still benefits (and still has 404s on 2.5.5) and it is faster as well (though the unpatched version is already a couple of seconds faster, so the gains are a little less.)

[Toflar]

🥳 by the way: you don't need to modify anything to log requests, it should be visible when running with -vvv 😊

[driskell]

🥳 by the way: you don't need to modify anything to log requests, it should be visible when running with -vvv 😊

Hahaha 😆 Yeh I don't know if I forgot about that or the output was too noisy and I was too lazy to grep but I'll definitely remember that 😊