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

Improve performance of DefaultPolicy #10585

Merged
merged 4 commits into from
Mar 15, 2022
Merged

Conversation

Toflar
Copy link
Contributor

@Toflar Toflar commented Mar 2, 2022

The $policy->selectPreferredPackages() is another bottleneck in the PoolOptimizer as we call it for every dependency group which can easily happen for 10 000+ times on medium size projects. Most packages match to a lot of dependency groups so we actually compare the same packages over and over again to sort them within one selectPreferredPackages() call. The respective 2 usort() calls were actually responsible for up to 25% of the whole PoolOptimizer::optimize() process.

With this PR we're now caching the result so the sorting can be way faster (another 0.5 - 1 seconds down on the PoolOptimizer).

Because we're using the PolicyInterface and Pool $pool is an argument, we have to make sure the cache is specific to the $pool instance. Thanks to the new PHP 7.2 requirement, we can use the faster spl_object_id() for that 馃槑

@Seldaek Seldaek added this to the 2.3 milestone Mar 15, 2022

foreach ($packages as &$nameLiterals) {
usort($nameLiterals, function ($a, $b) use ($pool, $requiredPackage): int {
return $this->compareByPriority($pool, $pool->literalToPackage($a), $pool->literalToPackage($b), $requiredPackage, true);
usort($nameLiterals, function ($a, $b) use ($pool, $requiredPackage, $poolId): int {
Copy link
Member

Choose a reason for hiding this comment

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

Have you tried caching the whole result like sort($nameLiterals); $cacheKey = implode(',', $nameLiterals); then memoize the whole usort() call instead of every individual comparison?

I am not entirely sure if the combinations of literals here will vary so much that it makes it useless or not.. But if not I guess it'd be even more efficient. Gotta try this with various projects though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think a complete match is pretty unlikely to happen. We compare within the dependency groups. So we have a group for e.g. ^4.2 and ^4.3 etc. Having the exact same packages inside those groups sounds very unrealistic to me. But I'll run some tests to see how many hits we'd have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out I was wrong :) There are actually also thousands of hits 馃コ Both caches have very high hit rates now. I had to fix the cache key of the sorting cache too. I forgot to consider $requiredPackage.

@Seldaek Seldaek merged commit f125fc1 into composer:main Mar 15, 2022
@Seldaek
Copy link
Member

Seldaek commented Mar 15, 2022

Thanks

@Toflar Toflar deleted the policy-performance branch March 15, 2022 14:39
emahorvat52 pushed a commit to emahorvat52/composer that referenced this pull request Feb 3, 2023
Add memoization to selectPreferredPackages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants