Do not unnecessarily load replaced packages into the pool #9619
Conversation
| @@ -399,7 +399,6 @@ private function loadPackage(Request $request, PackageInterface $package, $propa | |||
| if (isset($this->loadedPackages[$replace], $this->skippedLoad[$replace])) { | |||
| if ($request->getUpdateAllowTransitiveRootDependencies() || !$this->isRootRequire($request, $this->skippedLoad[$replace])) { | |||
| $this->unlockPackage($request, $replace); | |||
| $this->markPackageNameForLoading($request, $replace, $link->getConstraint()); | |||
There was a problem hiding this comment.
this means that this constraint won't be taken into account anymore if we then have a reason to load that package. Is this correct @naderman ?
There was a problem hiding this comment.
Absolutely need some insight on this. From what I can gather, this just means the REPLACED package isn't loaded. Which should be fine as there should still be a REQUIRE for it somewhere that causes it to load. Because of the skippedLoad check it only does the unlock if it was locked initially. And I think if a REQUIRE was hit, it would unlock it already.
Essentially I'm just trying to prevent composer loading packages that are never referenced anywhere except with a REPLACE. As with drupal/core-recommended the long list of replace pretty much don't exist. Ideally composer would only load a package referenced by REPLACE if it was also referenced by a REQUIRE. Since it loads everything in a REQUIRE anyway it means don't need to do anything for REPLACE. (I think!)
There was a problem hiding this comment.
@driskell but if later (or ealier) we decide to load the package due to a require rule, the constraint of the replace rule is now totally ignored. That's what I want to confirm whether it is safe or no.
There was a problem hiding this comment.
I can try to do some tests. I did find after removing this line that tests continued to pass with an exception of a handful where I couldn't see any impact based on what they tested. It would be great to get a test added for the scenarios this could break.
For your example, I did think, that if earlier we hit a require then skippedLoad would be empty as it would've been unlocked and loaded per the transitive dependencies flags.
This is where requires is explored on loaded packages:
// if the required package is loaded as a locked package only and hasn't had its deps analyzed
if (isset($this->skippedLoad[$require])) {
// if we're doing a full update or this is a partial update with transitive deps and we're currently
// looking at a package which needs to be updated we need to unlock the package we now know is a
// dependency of another package which we are trying to update, and then attempt to load it again
if ($propagateUpdate && $request->getUpdateAllowTransitiveDependencies()) {
if ($request->getUpdateAllowTransitiveRootDependencies() || !$this->isRootRequire($request, $this->skippedLoad[$require])) {
$this->unlockPackage($request, $require);
$this->markPackageNameForLoading($request, $require, $linkConstraint);
} elseif (!isset($this->updateAllowWarned[$this->skippedLoad[$require]])) {
$this->updateAllowWarned[$this->skippedLoad[$require]] = true;
$this->io->writeError('<warning>Dependency "'.$this->skippedLoad[$require].'" is also a root requirement. Package has not been listed as an update argument, so keeping locked at old version. Use --with-all-dependencies (-W) to include root dependencies.</warning>');
}
}
} else {
$this->markPackageNameForLoading($request, $require, $linkConstraint);
}
}
The subsequent unlock would've unset the skippedLoad, and thus the code in this PR would never run anyway, thus the constraint from the replace does not trigger any further package loads. Which makes perfect sense as a conflict: * should not load all package versions - it's the require that matters.
For the second scenario of later hitting the require - it was already unlocked by the code handling replace and skippedLoad is now empty - it then goes into the else above and just marks it for loading.
For the third scenario I'm thinking of now, where it hits the replace first, unlocks, then never sees a require, it would just not load it, and wouldn't need to as nothing requires what it is replacing.
I can try dig through existing tests see if the above are tested, or potentially maybe it is worth adding some new ones mentioning require/replace interactions explicitly and checking the above. That would at least check the above which is honestly just thoughts at this point that could still be missing something.
There was a problem hiding this comment.
I agree with what this is trying to do, but I do think this may cause bugs, so I'll try and come up with a test case for a scenario where this causes a problem so we can figure out a way to address that.
There was a problem hiding this comment.
I didn't completely follow but I'd say yes please open a PR for the change in load-replaced-package-if-replacer-dropped, if we can get that merged first maybe it'll help simplify this PR.
There was a problem hiding this comment.
I opened a PR for that branch with the test and what I think was the fix. #10410
So what's remaining for this PR I guess is to find a test for the below code or see what it's trying to do, as deleting it entirely (lines 468-489) currently still passes all tests, along with greatly improving the performance for Drupal site module updates due to preventing 404s when trying to load replaced packages that don't exist.
composer/src/Composer/DependencyResolver/PoolBuilder.php
Lines 468 to 489 in 24ce1ed
(NB: Happy New Year all 😄)
There was a problem hiding this comment.
If you rebase on main you'll notice that you cannot remove this code - it's been covered with some tests in the meantime :)
There was a problem hiding this comment.
And to add to that, the opposite case may be interesting too. Should upgrading a package to a version which now replaces a different locked package not result in that package getting unlocked/loaded and potentially removed? But I suppose the unlock call remains so that may still work as desired?
This is already covered in https://github.com/composer/composer/blob/main/tests/Composer/Test/Fixtures/installer/update-allow-list-with-dependencies-require-new-replace.test
There was a problem hiding this comment.
I think there may be another bug relating to multiple replacers replacing different versions of the same package and doing a partial update of it, but that's such an extrem edge case, and definitely already problematic in main, that I don't think it's worthwhile looking into now and further delaying this work here.
...ser/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixing-with-replacers.test
Show resolved
Hide resolved
Fixes Drupal drupal/core performance issue due to large number of replace declarations
Seldaek
force-pushed
the
avoid-unnecessary-loading-of-replaced-packages
branch
from
818c24d
to
60bd9b0
Compare
|
|
|
@stof see #9619 (comment) ? |
|
@stof @naderman Regarding comment #9619 (comment) I initially investigated this and found it was related to a different area of code: #9619 (comment) As per comment #9619 (comment) 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? composer/src/Composer/DependencyResolver/PoolBuilder.php Lines 468 to 489 in 24ce1ed |
|
👋 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!) |
|
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 ... |
|
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. ❤️ |
|
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 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 actually, your comment made me think about "why do we need I hope this makes it much more readable because all it is is basically not calling |
|
Same question for @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. 🥳 |
|
Historically we've only used released 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 |
:
|
I can confirm this is fixed by the changes in Profile results of the OP from running on Using commit 595559f on 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 |
|
🥳 |
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 😊 |
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 #9261 (comment)) 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 --profileThen install a module
composer require drupal/media_entity_twitter:2.5.0 --profileModify composer.json for media_entity_twitter to
^2.0Then update that module
composer update -W drupal/media_entity_twitter --profile(goes to 2.6.0 currently.)The timings I get are:
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:
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:
Savings of 20 seconds.
WDYT?
CC @Toflar after discussion in #9261 (Thanks!)