Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upbpo-39102: Increase Enum performance up to 10x times (3x average) #17669
Conversation
This comment has been minimized.
This comment has been minimized.
the-knights-who-say-ni
commented
Dec 20, 2019
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). CLA MissingOur records indicate the following people have not signed the CLA: For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. If you have recently signed the CLA, please wait at least one business day You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
super().__setitem__(key, value) | ||
|
||
@property |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
MrMrRobat
Dec 22, 2019
•
Author
I don't quite understand how it should look like, though. Should I change anything now?
Just realized that _value_ and _name_ attrs remained independent from name and value and could be different. This commit fixes it.
Run builtin python tests directly from python test module Refactor patch applying
Sorry a proper review is taking so long -- there is a lot to go over here.
I do have some private code that I haven't merged in yet that does away with Thank you for your efforts! |
This comment has been minimized.
This comment has been minimized.
Yeah, looks like I just deleted them by mistake. Fixed yesterday, pushed now :)
Oh, I get it. So I guess it will be fine to return
Does it mean that this PR won't be merged until your work is done?
Thanks, great to hear that :) Oh, and I have a question from https://bugs.python.org/issue39102, maybe it should be mentioned here:
def __invert__(self):
cls = self.__class__
members, uncovered = _decompose(cls, self._value_)
inverted = cls(0)
for m in cls:
if m not in members and not (m._value_ & self._value_):
inverted = inverted | m
return cls(inverted) |
@@ -369,7 +366,7 @@ def __members__(cls): | |||
return MappingProxyType(cls._member_map_) | |||
|
|||
def __repr__(cls): | |||
return "<enum %r>" % cls.__name__ | |||
return f'<enum {cls.__name__!r}' |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Benchmark: Testing with 10000 repeats, result is average of 100 tests: >>> str(~NewFlag.baz) # 5.6313761773697415 ms 'NewFlag.0' >>> str(~OldFlag.baz) # 146.97604789150913 ms 'OldFlag.0' >>> repr(~NewFlag.baz) # 4.422742834372443 ms '<NewFlag.0: 0>' >>> repr(~OldFlag.baz) # 151.49518529891247 ms '<OldFlag.0: 0>' >>> ~(~NewFlag.foo) # 3.465736084655711 ms <NewFlag.foo: 1> >>> ~(~OldFlag.foo) # 177.88357201820338 ms <OldFlag.foo: 1> NewFlag: total: 13.5198551 ms, average: 4.4194400 ms (Fastest) OldFlag: total: 476.3548052 ms, average: 158.2196475 ms, ~ x35.80 times slower than NewFlag
Feedback needed |
value = self._value_ | ||
if self.name is not None: | ||
return f're.{self.name}' | ||
value = self.value |
This comment has been minimized.
This comment has been minimized.
for k, v in c.__dict__.items() | ||
if isinstance(v, DynamicClassAttribute)} | ||
|
||
# Reverse value->name map for hashable values. | ||
enum_class._value2member_map_ = {} | ||
|
||
# used to speedup __str__, __repr__ and __invert__ calls when applicable |
This comment has been minimized.
This comment has been minimized.
MrMrRobat
Dec 25, 2019
Author
Despite the fact that this works pretty well and gives a great performance boost, I'm not sure that it was implemented right, so feedback is highly appreciable.
# TODO: Maybe remove try/except block and setting __context__ in this case? | ||
try: | ||
exc = None | ||
result = cls._missing_(value) | ||
except Exception as e: | ||
exc = e | ||
result = None | ||
# Huge boost for standard enum | ||
if cls._missing_ is Enum._missing_: | ||
raise | ||
else: | ||
e.__context__ = ValueError(f'{value!r} is not a valid {cls.__qualname__}') | ||
raise |
This comment has been minimized.
This comment has been minimized.
MrMrRobat
Dec 25, 2019
Author
With this try/except block code runs twice slower for non-enum values checks, then without it .
Do we really need to bother checking all errors and add ValueError
to context , knowing it will happen either in standard Enum._missing_
or in user-defined code? In fact, Flag._missing_
also raises same error as Enum._missing_
and handled and rerised too.
MrMrRobat commentedDec 20, 2019
•
edited
Increase Enum performance
https://bugs.python.org/issue39102
Partial benchmark result, to see more, check full benchmark result and source code
Full benchmark result and source code
Changes
Enum.__new__
EnumMeta.__getattr__
,Enum.name
and.value
in members__dict__
for faster accessEnum._name_
and._value_
, should use public.name
and.value
nowEnum._member_names_
with._unique_member_map_
for faster lookups and iteration_EmumMeta._member_names
and._last_values
with.members
mapping (deprecation warning on using old attrs)_str_
,_repr_
and_invert_
attrs to cache results of functions of the same dunder names (x35 speed boost) (948d3de)DynamicClassAttribute
without need to use slow__getattr__
https://bugs.python.org/issue39102