Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter impossible packages from the pool #9620

Merged
merged 4 commits into from Dec 8, 2021

Conversation

driskell
Copy link
Contributor

@driskell driskell commented Jan 17, 2021

I have noticed updates of some Drupal packages can be fairly slow. This seems to affect at varying levels. One module I found it reproduces with is if you have drupal/block_permissions installed. It appears to be due to the fact it has a requires of drupal/core ~8 and in the latest version drupal/core ^8 || ^9.

When processing the update of the module it then causes the drupal/core package to be downloaded and added to the pool in all versions since 8.0.0 all the way to 8.9 and then 9.0 all the way to 9.2, and all symfony packages that are depended on in-between those versions. This then generates a significant number of conflict rules.

Using the composer.json at https://github.com/drupal/recommended-project/blob/8.9.x/composer.json to reproduce. When editing with nano change the requirement on block_permissions to "^1.0".

$ composer install
...
$ composer require drupal/block_permissions:1.0.0
...
$ nano composer.json
...
$ composer -vvv update -W drupal/block_permissions --profile
[488.1MiB/27.72s] Dependency resolution completed in 0.436 seconds
[87.9MiB/27.84s] Analyzed 10213 packages to resolve dependencies
[87.9MiB/27.84s] Analyzed 736505 rules to resolve dependencies
...
[56.1MiB/28.01s] Memory usage: 56.06MiB (peak: 488.6MiB), time: 28.01s

As you can see, nearly 3 quarters of a million rules. In fact, on my project I'm having these issues with - there's around 990,000. The majority are conflict rules.

Specifically this means we can't use dependabot as effectively as we'd hoped - as each check of a module for updates takes a significant amount of time. The PR in #9619 reduces the time a large amount but it still is slow.

This PR aims to reduce the rule count - similar to the Pool Optimisation in #9261 (Maybe this should be a second optimisation rule that can be disabled?) - that aims updates only - and hopefully speed up dependabot and just general updates. It does it by taking the list of packages still fixed, and filtering the pool. This removes all the versions of drupal/core in the pool that do not meet the requirement in the composer.json that are still fixed. Thus reducing the package count substantially.

I do not believe problem output should be impacted as I think if a package did come in needing a different version of something, that would be unlocked by -w / -W if needed to allow that recalculation. Though I may have missed something so worth some eyes.

For me it reduces the time by a good 5 seconds for the above scenario. It also significantly reduces the memory usage to about a 10th of what it was.

$ composer -vvv update -W drupal/block_permissions --profile
[55.5MiB/23.68s] Dependency resolution completed in 0.012 seconds
[50.2MiB/23.68s] Analyzed 2450 packages to resolve dependencies
[50.2MiB/23.68s] Analyzed 5948 rules to resolve dependencies
...
[43.5MiB/23.84s] Memory usage: 43.53MiB (peak: 73.47MiB), time: 23.84s

When combined with the PR in #9619 it reduces timings and memory further.

Just PR 9619
[40.5MiB/8.13s] Memory usage: 40.48MiB (peak: 520.66MiB), time: 8.13s
Combined
[25.3MiB/6.42s] Memory usage: 25.25MiB (peak: 60.39MiB), time: 6.42s

CC @Toflar as this was based on my investigating drupal bits in the Pool Optimiser PR #9261.

(Background: trying to make Dependabot faster so we can use on Drupal effectively - GitHub's own hits 45 minute timeout for us - running ourselves it takes over an hour, about 75 minutes - with the above two PR it makes the full run less than 5 minutes and significantly less memory, bandwidth and CO2 馃槈 ... if it turns out is it working that is! )

@driskell
Copy link
Contributor Author

I plan to let someone look this over and decide where it could go - and if it does go somewhere I will then look at the lint failures. Tests should be working OK except where I used incompatible syntax.

@driskell
Copy link
Contributor Author

driskell commented Jan 19, 2021

Some additional timings / memory with above drupal/block_permissions example

With Pool Optimiser PR (9261). Great benefit, as it flattens many of the packages into small subset. As you can see rules are much much less.

[51.7MiB/28.65s] Analyzed 1820 packages to resolve dependencies
[51.7MiB/28.65s] Analyzed 19141 rules to resolve dependencies
[54.0MiB/29.09s] Memory usage: 54.04MiB (peak: 96.65MiB), time: 29.09s

With this PR. Slight improvement, as it completely removes many packages that don't meet the requirements of the remaining fixed packages. Of course, does not flatten those packages into subset that are still potentially valid candidates. It does reduce the rules even further though regardless.

[51.3MiB/23.17s] Analyzed 2471 packages to resolve dependencies
[51.3MiB/23.17s] Analyzed 5957 rules to resolve dependencies
[45.4MiB/23.55s] Memory usage: 45.42MiB (peak: 75.56MiB), time: 23.55s

With both merged together. Best of both. Really small package set now of only those that could be modified to meet the new version update requested, and within that set where constraints are identical across versions they're pre-determined the best candidates.

[46.6MiB/21.94s] Analyzed 832 packages to resolve dependencies
[46.6MiB/21.94s] Analyzed 2141 rules to resolve dependencies
[48.8MiB/22.29s] Memory usage: 48.84MiB (peak: 77.94MiB), time: 22.29s

Worth noting that the Pool Optimiser PR will greatly improve even installations. Where this only improves updates as it needs fixed packages to work from. Overall it should make projects like dependabot that run updates continuously much much quicker.

@stof
Copy link
Contributor

stof commented Jan 19, 2021

Does this PR provide any benefit compared to #9261 ? If not, I'd rather focus the effort on the pool optimizer PR.

@driskell
Copy link
Contributor Author

@stof It does provide a benefit - in lower memory and also quicker - at least on my local tests. The size of that benefit is not as substantial though, and only really impacts updates. I did wonder if it could be the second optimisation after #9261 adds the optimiser - of course if it is viable and no issues found with it.

I would agree that #9261 should be prioritised over this one, as it impacts install AND update, and is a significant improvement as it is. Maybe this one for later?

鈥ated rule count, especially around symfony requirements
@Seldaek
Copy link
Member

Seldaek commented Dec 7, 2021

Ok rebased and adapted the code a little. I think it looks safe enough to merge. Would appreciate some more reviews here though /cc @naderman @Toflar

@driskell
Copy link
Contributor Author

driskell commented Dec 7, 2021

@Seldaek Thank you 馃檹

@@ -75,7 +75,7 @@ public function optimize(Request $request, Pool $pool)
{
$this->prepare($request, $pool);

$optimizedPool = $this->optimizeByIdenticalDependencies($pool);
$optimizedPool = $this->optimizeByIdenticalDependencies($request, $pool);
Copy link
Contributor

@Toflar Toflar Dec 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the implementation looks good from a logical standpoint. Nitpick: I would just revisit the way it's implemented here. I designed it so that you would list multiple independent optimization steps here like so:

$pool = $this->optimizeByA($pool);
$pool = $this->optimizeByB($pool);
// etc

And the current one is not related to "optimzing by identical dependencies" so it should imho go into its own step.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I see that, but then IMO the way it's creating a new pool in every optimization pass is perhaps not so ideal. Anyway I'll have another look later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK f4e3121 fixes it but I removed the pool cleanup pass to be done outside of optimizeBy* so that it's only done once. Otherwise keeping track of all removed packages etc gets even messier.

@Seldaek Seldaek merged commit 8c8d9ef into composer:main Dec 8, 2021
@Seldaek
Copy link
Member

Seldaek commented Dec 8, 2021

Thanks @driskell !

Just FYI the other one probably will not make it for 2.2 as I'm still a bit unsure about the edge cases, but hopefully we can get it in at some point.

jrfnl added a commit to jdevalk/clicky that referenced this pull request Dec 23, 2021
Composer 2.2 contains a a bug/over-optimization, which causes the install with changed, but compatible requirements to error out on "conflicting" requirements.

This commit puts a work-around for this in place, which still tries to use the caching for the Composer install as much as possible.

It is recommended to revert this commit if/when the Composer bug has been fixed and a new version has been released.

Ref:
* Bug report: composer/composer#10394
* Probable cause: composer/composer#9620
* Similar fix in YoastSEO: Yoast/wordpress-seo@b399942
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants