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

New Multi Conflict Rule for transitive conflicts, to reduce memory use by rules #8424

Merged
merged 4 commits into from
Nov 12, 2019

Conversation

naderman
Copy link
Member

@naderman naderman commented Nov 12, 2019

A large part of rule memory is used up with rules representing transitive conflicts, or mutual exclusion. This PR implements a new type of rule representing a combination of all these individual rules into a single object, greatly reducing memory usage and consequently also improving performance.

This PR is an alternative approach to #8053 - the upside is that its approach is more generic than just different versions of one package name. At the same time the alternative approach in #8053 may reduce memory more effectively by getting rid of parts of the watch graph entirely. So will need to do some benchmarking and explore whether these two approaches can maybe even be combined.

Sylius example before (on 1.9 branch):

[492.1MiB/75.38s] Memory usage: 492.1MiB (peak: 2943.1MiB), time: 75.38s

After (on 2.0 branch combined with this PR):

[167.0MiB/34.28s] Memory usage: 166.97MiB (peak: 1552.14MiB), time: 34.28s

@naderman naderman added this to the 2.0-core milestone Nov 12, 2019
@naderman naderman self-assigned this Nov 12, 2019
@naderman
Copy link
Member Author

Alternative #8053 merged into 2.0 branch on the same sample:

[167.0MiB/107.44s] Memory usage: 166.96MiB (peak: 1627.68MiB), time: 107.44s

While the memory reduction on the latter seems close to this PR, the slowdown resulting from the new computation seems to be so significant that it's probably not worth exploring the dynamic nature of that change any further.

@naderman
Copy link
Member Author

Just to verify I reran the tests with the new 2.0 option COMPOSER_DISABLE_NETWORK=1 and the performance was hardly impacted, so definitely sticking with the solution in this PR.

@naderman naderman merged commit 7fc0cb0 into composer:2.0 Nov 12, 2019
@naderman naderman deleted the multi-conflict-rule branch November 12, 2019 22:19
@naderman naderman restored the multi-conflict-rule branch November 12, 2019 22:19
@naderman naderman mentioned this pull request Jan 31, 2020
@naderman naderman deleted the multi-conflict-rule branch October 23, 2020 20:07
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.

1 participant