Optimize the OptionKey class more #14248
Conversation
It's become a performance issue.
dcbaker
requested review from
mensinda,
xclaesse and
jpakkane
as code owners
dcbaker
force-pushed
the
submit/optionkey-performance
branch
from
7c914ca to
5778847
Compare
|
This will conflict with absolutely everything in the optionrefactor branch. :( |
|
I have zero idea why all of the test are failing in CI. They all pass 100% locally |
| @@ -106,12 +106,13 @@ class OptionKey: | |||
| internally easier to reason about and produce. | |||
| """ | |||
|
|
|||
| __slots__ = ['name', 'subproject', 'machine', '_hash'] | |||
| __slots__ = ['name', 'subproject', 'machine', '_hash', '_as_tuple'] | |||
There was a problem hiding this comment.
I'm really not sure about this one. Tuple representation is only used for comparison, and I doubt we really need to sort OptionKeys. Especially since we now use the hash for equality comparison.
| @@ -192,6 +193,7 @@ def __repr__(self) -> str: | |||
| return f'OptionKey({self.name!r}, {self.subproject!r}, {self.machine!r})' | |||
|
|
|||
| @classmethod | |||
| @lru_cache(None) | |||
There was a problem hiding this comment.
I don't think this has a big impact. It is probably only used for parsing options given by the command line.
There was a problem hiding this comment.
This gets used in a surprising number of places, many of them because we still are doing user input validation below the interpreter.
dcbaker
force-pushed
the
submit/optionkey-performance
branch
2 times, most recently
from
c13d2c0 to
c7ec44e
Compare
|
Shouldn't the hash only be used as a quick filter, only to then test the tuple if the hash matches? |
|
s/Optimze/Optimize/ in the PR title. |
dcbaker
changed the title
If the hashes match and they're not equal we have a problem, since that would mean that |
|
No, the hash collections always check with If you create a subclass of str which always returns 1 for hash, set and dict performance will certainly suck but they will give correct results. |
dcbaker
force-pushed
the
submit/optionkey-performance
branch
from
c7ec44e to
e43f21c
Compare
|
Okay, in that case the commit is bogus and we should just go back to the tuple == tuple thing |
Using lazy_property would be great, but since the OptionKey is immutable it doesn't work, so we'll just calculate it at initialization time. Because of all the caching that should be relatively fast.
Since OptionKeys are immutable, it's fine to cache this.
This doesn't use lru_cache because we can re-use the hash calculation in a cache miss by not doing so, which makes a cache miss faster.
It's now intended that the `factory()` method will be used when not using `from_string`, so there's no reason to make it convenient to use the main initializer, and passing the hash in is convenient for the factory method.
dcbaker
force-pushed
the
submit/optionkey-performance
branch
from
e43f21c to
b4b1f81
Compare
| # in a cache miss situation. | ||
| _hash = hash((name, subproject, machine)) | ||
| try: | ||
| return _OPTIONS_CACHE[_hash] |
There was a problem hiding this comment.
It is unlikely, but two objects can have the same hash but not be equal. I'm not sure that the cache lookup based on the hash only is a good idea. If this is done so many times that computing the hash becomes the bottleneck, maybe it is better to simply cache the has.
| @@ -192,6 +195,25 @@ def __str__(self) -> str: | |||
| def __repr__(self) -> str: | |||
| return f'OptionKey({self.name!r}, {self.subproject!r}, {self.machine!r})' | |||
|
|
|||
| @classmethod | |||
| def factory(cls, name: str, subproject: str = '', machine: MachineChoice = MachineChoice.HOST) -> OptionKey: | |||
There was a problem hiding this comment.
I haven't tried to implement it, but I think you can obtain the same effect without forcing the users to call the .factory() class method by implementing __new__() instead.
| # the _type option to the constructor is kinda private. We want to be | ||
| # able to save the state and avoid the lookup function when | ||
| # pickling/unpickling, but we need to be able to calculate it when | ||
| # constructing a new OptionKey | ||
| object.__setattr__(self, 'name', name) | ||
| object.__setattr__(self, 'subproject', subproject) | ||
| object.__setattr__(self, 'machine', machine) | ||
| object.__setattr__(self, '_hash', hash((name, subproject, machine))) | ||
| object.__setattr__(self, '_hash', hash_ if hash_ is not None else hash((name, subproject, machine))) | ||
| object.__setattr__(self, '_as_tuple', (self.subproject, self.machine, self.name)) |
There was a problem hiding this comment.
This starts to look very very very similar to a named tuple 🙂
|
@bonzini is right about hash collisions. Sorry for guiding you in the wrong direction... Like @dnicolodi said, it is possible to implement the cache without breaking the API, using the |
These changes attempt to make the OptionKey faster to initializer and to compare.
with some light testing this makes initializing options keys roughly 5x as fast (when initializing the same key many times) and comparing two pre-initialized keys roughly twice as fast.