Optimize MultiConstraint

[jderusse]

This PR optimize the MultiConstraint by alowing more than 2 constraints:

^1.0 || ^2.0 || ^3.0 => >= 1.0 <4.0

It also replace calls to MultiConstraint methods by accessing private properties (allowed because same class)

[GrahamCampbell]

Should add a test with ^0.1 || ^1.0 || ^2.0 to test that is NOT collapsed.

[jderusse]

Should add a test with ^0.1 || ^1.0 || ^2.0 to test that is NOT collapsed

You mean ^0.1 || ^1.0 should not be collapsed, but ^1.0 || ^2.0 shoud be collapsed.

[jderusse]

I added new cases to also merge multiple other constraints

^1.0 != 1.0.1 || ^2.0 !=2.0.1 || ^3.0 || ^4.0 != 4.0.1

# before
Multiple:
 Multiple: >= 1.0 <2.0 != 1.0.1
 Multiple: >= 2.0 <3.0 != 2.0.1
 Multiple: >= 3.0 <4.0
 Multiple: >= 4.0 <5.0 != 4.0.1

# after
Multiple: >= 1.0 <5.0 != 1.0.1 != 2.0.1 != 4.0.1

[jderusse]

NOTE: this PR does not try to optimize user's mistake (range overlaps): It would have a cost at runtime for something that should not happens:

ie: ^1.0 || = 1.0, ^1.1 || >1.0...

[codecov-io]

Codecov Report

Merging #84 into master will decrease coverage by 0.60%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master      #84      +/-   ##
============================================
- Coverage     96.03%   95.42%   -0.61%     
- Complexity      250      257       +7     
============================================
  Files             7        7              
  Lines           479      503      +24     
============================================
+ Hits            460      480      +20     
- Misses           19       23       +4     

Impacted Files	Coverage Δ	Complexity Δ
src/Constraint/MultiConstraint.php	93.20% <100.00%> (-3.00%)	53.00 <19.00> (+7.00)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ec124f...d7b967b. Read the comment docs.

[Toflar]

Great to see someone picking up where I left off :)

NOTE: this PR does not try to optimize user's mistake (range overlaps): It would have a cost at runtime for something that should not happens:

Unfortunately, that‘s not an option because it‘s also used internally by Composer itself.
It‘s important it deals properly with everything...

[Toflar]

I had this on my list anyway but maybe you wanna give it a shot: We might want to use lower and upper constraint bounds for optimizing them :)

[jderusse]

I think you misunderstood what's I'm saying.

Such case will still works, but it will not be optimized. In short, it will not be better nor worse than the current implementation.

[jderusse]

We might want to use lower and upper constraint bounds for optimizing them :)

I thought about that but did not get the expected results...
In fact the optimization is not cached and optimization is performed on each call... The footprint of this optimization pass should be as low as possible
Moreover such case is quite rare: most of the case are like ^1.0 || ^2.0.

Without cache, optimizing such case reduce overall performances :-(

opportunity for a compile method that generate a php file that will be evaluated at runtime??

[Toflar]

I think you misunderstood what's I'm saying.

Ah, good then :)

And yes, compilation would be great, that‘s why there‘s #71 :)

[Seldaek]

Looks good to me code wise but do you have any numbers on a few composer runs what the impact is? I'm just worried that too intensive optimization leads to higher CPU time for only a minor memory gain.. but hopefully that's not the case.

[jderusse]

The gain is not about memory, It's also about CPU: It reduces the number of constraint evaluate

Here is a comparison of for magento (-5% cpu): https://blackfire.io/profiles/compare/3897a2dc-7956-40b6-9118-204906544244/graph (reduce from 382,940 to 369,053 calls to version_compare (included Constraint:match, Constraint:matchSpecific...)

And antorher on a personal project: https://blackfire.io/profiles/compare/dc399174-be9e-4bec-9046-21b1f376c448/graph

[naderman]

That looks really cool @jderusse - did you take a look at actually dealing with overlaps in the constraints? This isn't really relevant right now, but @Toflar and I are considering options to optimize building the package pool in Composer and would need to merge constraints for the same package from multiple require statements in other packes, so they are likely to overlap. If there's more of a performance hit for that, we could make it a separate function too, which would only be called in that specific context?

[jderusse]

@naderman overlaps will improve result much more. But:

• computing overlaps is costly
• overlaps is rare
  At the end, using overlap to reduce MultiConstraint does not worth it. CPU spent much time to compute overlaps, than benefit from optimization.

That would not be an issue if optimization were cached (which is not the case for the moment).
I'm currently writing a compile method to generate optimized code that could be cached.

[naderman]

I don't think you read all of what I wrote? We're talking about a future change in Composer that would result in overlap not being rare anymore at all. Like I said a separate function to apply this optimization only for the use case where overlap is common seems alright to me?

[Toflar]

I'm currently writing a compile method to generate optimized code that could be cached.

<3

[staabm]

Lgtm - love it!

[staabm]

Lets get this baby landed :). Well done!

[Seldaek]

Seems like this doesn't do much in terms of CPU/memory change (I see a tiny bit less memory, and CPU probably too but so small it's hard to tell if relevant).

That said, if there is an impact on larger projects that's good, and anyway it's a good improvement code wise! Thanks.

[Seldaek]

And as @naderman said, if you are interested in working on optimization of overlapping constraints as a separate util class, that would probably be valuable to have too :)