Perf: Skip redundant creation of Versions in specifier comparison #986
Conversation
| return version | ||
|
|
||
|
|
||
| def _public_version(version: Version) -> Version: |
There was a problem hiding this comment.
I feel like this would be cleaner if it was part of version; .local_version could return self if there is no local version, for example; it's a bit of an issue that these returned strings requiring reparsing. But unfortunately, .base_version already has the word "version" in it but returns a string, so not sure there's an obvious way forward with naming if we went that route.
There was a problem hiding this comment.
Designing a new API for Version objects to return Version objects doesn't need to block this performance improvement though. We can migrate this to new API once that lands.
I am working on a .replace() method, and I hadn't thought about it, but I can efficiently return self when nothing is modified. That could be the target API.
There was a problem hiding this comment.
No, not blocking, already approved. :)
There was a problem hiding this comment.
(Actually, I approved #986, which I felt was sort of a prerequisite, but I guess it isn't.)
There was a problem hiding this comment.
Yeah, the two PRs are technically independent of each other, they just touch some of the exact same lines, I will need to resolve conflicts once one or the other merges, no issue though, I already know the target state.
When doing a lot of comparisons with the
Specifierclass, a lot of redundant temporaryVersionobjects are created for comparison.This is particularly problematic for pip, which has to call comparisons many times, on one of my resolver benchmark, with #985 already applied, adding the
_public_versionfunction reduces the number of timesVersionis instantiated from ~1.3 million times to ~580k times, and the_base_versionfunction reduces that to ~574k times (_base_versionis called in significantly more edge case situations).