Perf: Cache Specifier Version #985
Conversation
henryiii
left a comment
There was a problem hiding this comment.
I'm guessing that it's not possible to just cache this and allow it to be used directly, due to the fact the _compare methods, which take a string, are how it gets used?
| # version in the spec. | ||
| return True | ||
|
|
||
| def _compare_greater_than(self, prospective: Version, spec_str: str) -> bool: |
There was a problem hiding this comment.
Are these _compare_* methods used somewhere, or are they considered public? I don't see any usage of them.
There was a problem hiding this comment.
The compare methods are not public, they are called dynamically via the _get_operator method. I do not like this internal API design, it means the compare methods aren't correctly type checked.
But significantly refactoring the internal API design and makng a small optimization should be separate PRs. And I'm not going to propose an internal API redesign any time soon.
I'm guessing that it's not possible to just cache this
Yeah the API design prevents this, and _compare_compatible makes use of the fact a different version can be passed to the operator than the specification version, and _compare_arbitary make use of the fact it's a string not a Version object that's passed around.
This PR was the result of me trying to add a simple internal property cache, realizing that required a large refactor to the API design and had awkward edge cases, and then arriving at this solution.
One problem with a
Specifieris that if you call it's methods more than once it has to keep recreating the underlyingVersionfor the specifier.This is particularly problematic for pip, which does exactly this, on one of my resolver benchmark this reduces the number of times
Versionis instantiated from ~3.5 million times to ~2.5 million times, providing a non-trivial improvement in CPU time.This is the most elegant solution I could find given the
Specifierinternal API.