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

Use CompilingMatcher in DefaultPolicy for performance reasons #11638

Merged
merged 2 commits into from
Sep 27, 2023

Conversation

Toflar
Copy link
Contributor

@Toflar Toflar commented Sep 13, 2023

This needs more investigation for sure but I wanted to leave it here so others can investigate too.

I noticed that in the DefaultPolicy we do not use the CompilingMatcher which causes all constraints to be dynamically evaluated over and over again. Since the introduction of the PoolOptimizer, the DefaultPolicy is heavily used to select the correct packages within the dependency groups.

I did a few tests and this reduces the PoolOptimizer runtime by about 10%. The more packages in the pool, the higher the potential savings. In absolute times we're talking 0.2 - 0.5 seconds, maybe.

However, in order to make the tests pass, I had to add some dev- comparison which I'm a bit confused with. Things to clarify:

  • Why is there Constraint::matchSpecific() and why is this a public method and not an implementation detail?
  • Why does the CompilingMatcher handle this case differently - is this a bug in composer/semver?

@Seldaek
Copy link
Member

Seldaek commented Sep 26, 2023

The problem is we do treat branches differently here for ordering purposes than for normal constraint matches, hence you have matchSpecific with the second compareBranches: true param.

So IMO the only correct way is to keep doing this for branches and skip it otherwise.

I don't know if this negates the perf gains but I would imagine it still helps a bit? Can you check?

@Toflar
Copy link
Contributor Author

Toflar commented Sep 26, 2023

It sure does. Here's how you can easily compare: composer update --dry-run -vvv --profile 2>&1 | grep optimize

main@755de04bf

[101.8MiB/3.93s] Running pool optimizer.
[142.4MiB/7.70s] Pool optimizer completed in 3.767 seconds
[142.4MiB/7.70s] Found 20,252 package versions referenced in your dependency graph. 17,241 (85%) were optimized away.

This PR@6d3db22

[101.7MiB/3.99s] Running pool optimizer.
[144.4MiB/7.44s] Pool optimizer completed in 3.457 seconds
[144.4MiB/7.44s] Found 20,252 package versions referenced in your dependency graph. 17,241 (85%) were optimized away.

I think that's an easy win for everybody 😊

$constraint = new Constraint($operator, $b->getVersion());
$version = new Constraint('==', $a->getVersion());
// dev versions need to be compared as branches via matchSpecific's special treatment, the rest can be optimized with compiling matcher
if (strpos($a->getVersion(), 'dev-') === 0 || strpos($b->getVersion(), 'dev-') === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need to worry about versions which end with -dev?

https://getcomposer.org/doc/articles/versions.md#branches

In the case of a branch with a version-like name (v1, in this case), you append -dev as a suffix, rather than using dev- as a prefix.

Copy link
Member

Choose a reason for hiding this comment

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

No because those get normalized like e.g v1 branch becomes 1.9999999.9999999.9999999-dev and that plays well with version compare. The problem is really named branches which aren't versions at all.

@Seldaek
Copy link
Member

Seldaek commented Sep 27, 2023

Alright then let's get this in, thanks!

@Seldaek Seldaek merged commit f6ce834 into composer:main Sep 27, 2023
20 checks passed
@Seldaek Seldaek added this to the 2.6 milestone Sep 27, 2023
@naderman
Copy link
Member

naderman commented Oct 4, 2023

@Toflar thanks! 🥳

@composer composer deleted a comment from Griffin641498 Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants