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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize performance by filtering dependencies early #8295

Open
wants to merge 1 commit into
base: 2.0
from

Conversation

@ausi
Copy link

ausi commented Aug 27, 2019

Inspired by @Toflar’s #8275 this pull request tries to skip loading for packages that conflict with the root requirements. But after every loading loop iteration in the PoolBuilder it extends the root requirements with new information.

This can be done because if all versions of a root dependency require the same package, this package can become a root dependency itself with a union of all version constraints. For example:

  1. The root composer.json requires package A in version ‌^2.0
  2. The matching versions 2.0 and 2.1 get loaded, all lower versions get skipped.
  3. A 2.0 itself requires package B in ‌^3.0 and A 2.1 requires B in ^3.1 therefore we can now add a root requirement for B ^3.0‌
  4. Now when package B gets loaded all versions not matching ^3.0‌ get skipped directly.

I tested the version of my pull request and the results look very promising I think:

1.9.0 2.0-dev
2019-08-02
this pull request
"symfony/framework-bundle": "^3.4" 7.14 s
265.14 MB
3.82 s
123.84 MB
2.78 s
67.03 MB
"contao/managed-edition": "^4.8" 33.10 s
1618.04 MB
87.53 s
2802.62 MB
8.02 s
126.56 MB
"contao/core-bundle": "^4.8" 33.25 s
1142.62 MB
68.31 s
2569.67 MB
7.16 s
77.73 MB
"contao/core-bundle": "*" 39.55 s
1615.29 MB
83.93 s
2804.13 MB
78.83 s
2002.55 MB

As you can see in the last example: if the requirement is very unrestricted the benefits aren’t that great, but I think in many real world use cases this change could make a huge impact.

The concept is pretty much the same as #8275 but without the need for manually adding root requires or conflicts.

The state of this pull request is just a first draft, if you like the concept I would continue working on it:

  • Support replaces and provide.
  • Support aliases.
  • Add optimization to reduce the large number of constraints in the MultiConstraint objects.
  • Update loadPackages method signature.
  • Fix tests.
$count = 0;
$subRequireConstraints = array();
foreach($this->packages as $package) {
if ($package->getName() === $name && $constraint->matches(new Constraint('==', $package->getVersion()))) {

This comment has been minimized.

Copy link
@stof

stof Aug 28, 2019

Contributor

how does this deal with provide and replace (i.e. packages having multiple names) ?

This comment has been minimized.

Copy link
@ausi

ausi Aug 28, 2019

Author

The pull request is not yet finished. ☺️
Support for provide and replace is on my ToDo list in the issue description.

Copy link
Contributor

Toflar left a comment

Yeyy! Great to see my approach gets another try at v2.0! 🎉
The concept looks correct to me and should definitely be continued 👍

private function optimizeConstraints(array $constraints)
{
// TODO: constraints can possibly be optimized even more, maybe this should go into the MultiConstraint class?

This comment has been minimized.

Copy link
@Toflar

Toflar Aug 28, 2019

Contributor

I think it should go to MutliConstraint, yes. However, we have to make sure that the pretty version is not changed as this could cause confusion when information is shown to the user. :)

This comment has been minimized.

Copy link
@ausi

ausi Aug 28, 2019

Author

These constraints aren’t shown to users so the pretty string shouldn’t be an issue here.

@Toflar

This comment has been minimized.

Copy link
Contributor

Toflar commented Aug 28, 2019

I wonder why v2.0 is so much slower than 1.9 without your PR in some cases 🤔

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Aug 28, 2019

Definitely looks like a good way to go to me, but tbh I'd wait before investing more time in this until #7936 gets completed as it's already huge and risks conflicting with a lot of things.

@Seldaek Seldaek added this to the 2.0-core milestone Aug 28, 2019
@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Aug 28, 2019

As for why 2.0 uses so much more memory and time than 1.x, that is indeed odd and should be investigated eventually but we also aren't there yet I think..

@ausi

This comment has been minimized.

Copy link
Author

ausi commented Aug 28, 2019

Thanks for the feedback. I’ll leave this PR as it is then for now.

I subscribed to #7936 so that I can continue working once it got merged. Otherwise feel free to ping me once you think 2.0 is ready for such pull requests ☺️

@andrerom

This comment has been minimized.

Copy link
Contributor

andrerom commented Dec 1, 2019

Seeing #7936 and follow ups having been merged into 2.0 now

Whats the state of this after #8424 was merged? Not applicable or still relevant?

@ausi

This comment has been minimized.

Copy link
Author

ausi commented Dec 1, 2019

I will have to take another look at this, rebase the PR and compare the performance.

@Toflar

This comment has been minimized.

Copy link
Contributor

Toflar commented Dec 2, 2019

I already did :) It's still very relevant but the current 2.0 branch still needs some updates. Optimizations should go to the new PoolBuilder. Let's work on this together, ping me on Slack :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.