perf: cache dependency names in conflict checks#14088
Conversation
4b6aa10 to
a12bcc1
Compare
|
Sorry for the force push, I'm still getting my workflow under control |
|
Nah, it's fine as long no has reviewed your PR yet. It's just after someone has reviewed and you've pushed changes in response that force-pushes make life harder. |
|
@ichard26 ah ok, thanks! I saw the CPython docs mention they dont like force pushes, I keep forgetting this is sort of a separate entity, the lines get blurry |
While often confused as being part of CPython, pip is organizationally, developmentally, and culturally an entirely independent project. The main connection is that pip is vendored into CPython (but many libraries/tools are) and that historically pip maintainers have also been core Python developers, but this is less so now. |
|
@notatallshaw thanks for the explanation, yeah I'm starting to see the distinction, I'm sad to see that and I don't think that should be the case, but I'll approach that later on :) |
There was a problem hiding this comment.
I reviewed the logic and it seems good from a correctness perspective but my bench marking showed this PR was slower on the scenario I tested compared to main, to proceed with this PR you need fix and provide a real performance improvement. Here was my method and findings:
I compared the operations/check.py functions on main against this PR. The input is a generated graph of 351 packages (~7.9 dependencies each, ~2,800 edges) with names in non-canonical form so canonicalize_name does real work; one hub is depended on by ~80% of the rest, giving an affected-package whitelist of ~285 so the install path exercises the loops this PR changes.
I timed two paths:
- pip check: build the package set, then
check_package_set. - install conflict: build, build the whitelist, then
check_package_setrestricted to it.
Functions are fed pre-parsed Requirement objects, so the timing covers the conflict-check computation and excludes metadata reading (identical on both branches). The harness is paired: each batch times both variants back to back in randomized order, 150 batches, asserting identical results first. I ran it on CPython 3.15.0b2 and 3.13.12 and checked the breakdown with the 3.15 sampling profiler.
Results
One run on 3.15.0b2:
packages: 351, dependency edges: 2786, Exp2 whitelist: 285
Exp1 pip check path (build + check, no ignore)
baseline : 2318.43 us/run (median 2287.58)
PR : 2412.22 us/run (median 2392.60)
PR minus base : +93.79 us (+4.05%)
PR faster in : 8/150 batches
paired t-test : t=-15.02 p=5.30e-51
sign test : z=-10.94 p=7.33e-28
Exp2 install conflict path (build + whitelist + check)
baseline : 2149.23 us/run (median 2134.17)
PR : 2168.70 us/run (median 2159.35)
PR minus base : +19.47 us (+0.91%)
PR faster in : 27/150 batches
paired t-test : t=-2.74 p=6.14e-03
sign test : z=-7.84 p=4.56e-15
A positive "PR minus base" means the PR is slower. Across several runs on both versions:
| Python | pip check path | install conflict path |
|---|---|---|
| 3.15.0b2 | +4.6% to +5.7% | +0.9% to +3.5% |
| 3.13.12 | +5.3% | +2.7% |
Significant in every run (p well below 0.05); the PR was faster in 0 to 27 of 150 batches, about 0.1 ms per call on this set.
Sampling profiler on the pip check path:
- main:
SpecifierSet.contains~36% of samples,canonicalize_name~8%. - this PR:
canonicalize_name~15% (now in set construction, not the check loop),SpecifierSet.contains~24%, and thezip(..., strict=True)loop the top line at ~31%.
It looks like the overhead of this PRs construction outweighs any performance gains.
|
@KRRT7 do you want pick this up or not? |
Yeah, forgot about it, will work on it soon |
|
I'm just going to move it to draft for now, so it's easier for me to see which PRs are not yet ready for review. Feel free to change the status any time. |
Depends on #14075 being merged first.
This PR keeps the whitelist scope unchanged and caches normalized dependency names to reduce repeated canonicalization in conflict-check paths.