bpo-39102: Increase Enum performance up to 10x times (3x average) #17669

Bobronium · 2019-12-20T03:01:56Z

Increase Enum performance

https://bugs.python.org/issue39102

Partial benchmark result, to see more, check full benchmark result and source code

ATTR_ACCESS:
Testing with 10000 repeats, result is average of 10 tests:

    >>> NewEnum.foo  # 0.6069349023270748 ms
    <NewEnum.foo: 1>

    >>> OldEnum.foo  # 4.5089948174334005 ms
    <OldEnum.foo: 1>

    >>> NewEnum.foo.value  # 0.965516420197984 ms
    1

    >>> OldEnum.foo.value  # 5.234090615261106 ms
    1

    >>> NewEnum.value.value  # 0.6496817059283957 ms
    3

    >>> OldEnum.value.value  # 19.718928336346387 ms
    3

    >>> try:     NewEnum.value.value = 'new' except: pass  # 4.842046525117963 ms
    AttributeError("Can't set attribute")

    >>> try:     OldEnum.value.value = 'new' except: pass  # 24.484108522222897 ms
    AttributeError("can't set attribute")


NewEnum: total: 7.0641796 ms, average: 1.1652198 ms (Fastest)
OldEnum: total: 53.9461223 ms, average: 10.3317085 ms, ~ x8.87 times slower than NewEnum

Full benchmark result and source code

Changes

Optimized Enum.__new__
Remove EnumMeta.__getattr__,
Store Enum.name and .value in members __dict__ for faster access
Deprecate getting values from Enum._name_ and ._value_, should use public .name and .value now
Replace Enum._member_names_ with ._unique_member_map_ for faster lookups and iteration
Replace _EmumMeta._member_names and ._last_values with .members mapping (deprecation warning on using old attrs)
Add _str_, _repr_ and _invert_ attrs to cache results of functions of the same dunder names (x35 speed boost) (948d3de)
Various minor improvements
Add support for direct setting and getting class attrs on DynamicClassAttribute without need to use slow __getattr__

https://bugs.python.org/issue39102

…mpt)

# Conflicts: # Lib/enum.py

the-knights-who-say-ni · 2019-12-20T03:01:59Z

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 Missing

Our records indicate the following people have not signed the CLA:

@MrMrRobat

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
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

…m member

auvipy · 2019-12-22T04:44:31Z

Lib/enum.py

        super().__setitem__(key, value)

+    @property


the deprecation part could be addressed in a separate commit.

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

ethanfurman

Sorry a proper review is taking so long -- there is a lot to go over here.
What I can say so far:

name and value must show up in a member's dir();
_leading_underscore_names and plain_names must not be used in EnumMeta nor Enum as then the user cannot use those same names -- that's why _sunder_ and __dunder__ names are used instead;

I do have some private code that I haven't merged in yet that does away with DynamicClassAttribute -- I'll try to get that done in the next few weeks. I think a lot of the work you have done here will still be applicable.

Thank you for your efforts!

Bobronium

Feedback needed

Bobronium · 2019-12-25T16:44:17Z

Lib/re.py

-        value = self._value_
+        if self.name is not None:
+            return f're.{self.name}'
+        value = self.value


Should use _repr_ cache here

Bobronium · 2019-12-25T16:49:37Z

Lib/enum.py

                              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


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.

Bobronium · 2019-12-25T17:19:15Z

Lib/enum.py

+        # 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


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.

Bobronium · 2020-05-26T15:05:37Z

@ethanfurman, any chance this PR will be reviewed in the near future?

TylerYep · 2020-07-31T22:32:08Z

Not a core team member, but I would really love this performance boost - any chance this will be reviewed soon?

ethanfurman · 2020-07-31T22:55:45Z

I have some other changes planned, which include some speed ups. We'll see the changes in 3.10 (3.9 is in bug-fix only prior to first release).

Bobronium · 2020-08-01T00:05:40Z

I would really love this performance boost

@TylerYep, you can check out enum patch I've created for current python versions, it’s a drop-in solution that enables changes from this PR along with some other improvements: https://github.com/MrMrRobat/fastenum,
I’d appreciated any feedback on this :)

looks like I haven’t uploaded it on PyPi though. Maybe I’ll upload it on this weekend, but for now it’s still should be installable via pip:

pip install git+https://github.com/MrMrRobat/fastenum.git

belm0 · 2020-11-23T01:46:55Z

This PR takes the approach of caching things, perhaps that is best.

However, when I looked at the Flag code several months ago (being unaware of this PR), I kept wondering why carry such a heavy and complicated _decompose() function in the first place? A flag is just bits, right? So I sought to eliminate it.

Proof of concept is here: belm0#1

However, this refactor is thwarted by some odd edge behavior of Flag and IntFlag, which in my opinion no one should be relying on. To proceed with it, there would have to be will among maintainers to fix/deprecate these things:

repr of a compound value contains redundant compound values (e.g. given {A=1, B=2, C=4, AB=A|B}, repr(A|B|C) is C|AB|B|A)
allowing incoherent Flag definitions which don't decompose into individual bits (e.g. {B=3, C=4})
IntFlag invert behavior, which has a one-to-many mapping into the negative value space (e.g. given {A=1, B=2}, ~B is <Bar.A: -3> rather than <Bar.A: 1> (and furthermore ~B == A is False)

ethanfurman · 2020-11-23T02:44:35Z

@belm0 Thanks for the concise summary.

1 is a bug, and needs to be changed.
2 needs to stay.
3 needs both -- a Flag should behave as you say, but IntFlag, because it is based on integers, needs to behave like an integer would.

belm0 · 2020-11-23T04:07:35Z

3 needs both -- a Flag should behave as you say, but IntFlag, because it is based on integers, needs to behave like an integer would.

It was a design choice, wasn't it? IntFlag derives from int and Flag. When implementing invert, it was decided to make it act like int. It doesn't need to be that way, and I suspect most use cases would rather have the Flag behavior.

From my understanding, the primary motivation ofIntFlag was to have a Flag that could be passed into legacy or 3rd party API. Int-ness should have been secondary to Flag-ness. If there is some odd API that takes such a value and does operations other than &, |, ^, or expects~to act as a signed invert, go ahead and cast to int() before passing it in.

ethanfurman · 2020-11-23T18:31:45Z

@belm0 If a concrete data type is mixed into an Enum (such as str, int, CustomDataType, etc.) that data type is the most important thing about it. It would be more accurate to say "if an Enum is mixed into a concrete data type".

The primary motivation behind IntFlag was to be able to add Enum representation to "magic ints" that are already being used as flags -- so IntFlag behavior has to exactly match (or as nearly as possible) the behavior that an int would show.

What would be most helpful at this point is some actual code samples of "magic" integer flags that have inversion applied to them, and looking at how the surrounding code is treating the result of that operation.

Hmmm.... - just thinking out loud here, but it seems to me that the problem is that integer flags are (usually? always?) unsigned, but Python doesn't have that particular concept. Perhaps the thing to do is to make Flag reveal it's int roots (so they could be used as indices, in calculations, etc.) but have it represent the unsigned version, and IntFlag could then remain the signed version.

Thoughts?

belm0 · 2020-11-24T04:39:04Z

@belm0 If a concrete data type is mixed into an Enum (such as str, int, CustomDataType, etc.) that data type is the most important thing about it. It would be more accurate to say "if an Enum is mixed into a concrete data type".

The primary motivation behind IntFlag was to be able to add Enum representation to "magic ints" that are already being used as flags -- so IntFlag behavior has to exactly match (or as nearly as possible) the behavior that an int would show.

I don't agree with these rationales-- it probably won't be productive for us to argue it now.

What would be most helpful at this point is some actual code samples of "magic" integer flags that have inversion applied to them, and looking at how the surrounding code is treating the result of that operation.

Hmmm.... - just thinking out loud here, but it seems to me that the problem is that integer flags are (usually? always?) unsigned, but Python doesn't have that particular concept. Perhaps the thing to do is to make Flag reveal it's int roots (so they could be used as indices, in calculations, etc.) but have it represent the unsigned version, and IntFlag could then remain the signed version.

The only reasonable way to do flag negation with signed integers is to negate and then mask with the valid bits. This is effectively what Flag does, and it's what IntFlag should do as well. Either no one is using IntFlag invert, or if they are they're having to do such masking manually.

No one wants or is expecting a flag/enum component to have multiple representations (<Bar.A: 1>, <Bar.A: -3>, <Bar.A: -5>, ...).

ethanfurman · 2020-11-25T16:57:11Z

@belm0 : As I alluded to earlier, IntEnum needs to be a drop-in replacement for currently in-use magic ints -- and all the locations I looked at in the stdlib do indeed mask off the negative bits after an inversion.

As you say, it is the only reasonable way to use negated flags.

I'm more comfortable making that change now, thank you for your feedback.

belm0 · 2020-12-01T01:43:02Z

allowing incoherent Flag definitions which don't decompose into individual bits (e.g. {B=3, C=4})

2 needs to stay.

What is the use case for this one?

Given something like {B=3, C=4}, what will be the defined behavior of the new iteration added to 3.10?
What about {A=1, B=3, C=4}? (the iteration is expected to produce individual bit members)

ethanfurman

For 3.10 and supporting __init_subclass__ I've made several structural changes to the enum implementation. In doing that I included many of the optimizations that you spoke of. If you are not already in the ACK file I'll add you there in my next commit.

Improvements made since your PR:

enum.property, a subclass of DynamicClassAttribute, is used : it avoids the slow __getattr__ lookup
decompose has been removed (with belm0's help)
_invert_ is cached

Things that likely will not change:

_name_ as an enum.property
_value_ as an enum.property
__member_map__ as the public API for accessing enum internals

Things I am open to:

using set() instead of lists
using internal dict()s instead of matching list()s to track names/values
using f-strings
caching __member_map__ instead of creating a new one on each access

If you would like to make another PR based on current 3.10 code based on above comments I would be happy to review and commit it. If you don't have time or the desire to tackle this again, you still have my thanks for the ideas and your help in the improvements that have made it in so far.

bedevere-bot · 2021-04-11T15:40:18Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

belm0 · 2021-04-11T23:39:33Z

@MrMrRobat would you mind running your enum benchmark against head?

https://gist.github.com/MrMrRobat/94b5ca3e4098669d93a6127e70359307

ethanfurman · 2021-04-12T01:42:08Z

Creating of enums got slower with the extra dance I had to do to support __init_subclass__ properly, but using enum should be a bit faster now.

Bobronium · 2021-04-12T07:33:26Z

@ethanfurman, thanks for the feedback! Should I open another PR, or will it be enough to just merge master changes into this one?

__member_map__ as the public API for accessing enum internals

caching __member_map__ instead of creating a new one on each access

I don't quite understand what member map you are referring to, could you please clarify?

Bobronium · 2021-04-12T07:34:29Z

@belm0, sure, I will.

ethanfurman · 2021-04-12T15:47:51Z

@MrMrRobat Sorry, I meant __members__. I think it would be easier to make a new patch based on the 3.10 code, and force push to this PR. A lot has changed, and many of the improvements you have here are either already incorporated or not appropriate -- for example, _name_ and _value_ being accessed via descriptors is that way to provide some protection against modifying them, and that's not going to change.

Bobronium added 7 commits August 9, 2019 14:11

Added class attributes access trough DynamicClassProperty (first atte…

cad12cc

…mpt)

Added class attributes access trough DynamicClassProperty (first atte…

d6a48c3

…mpt)

Replaced dynamic attributes Enum.name and .value to instance attrs

923902e

Improve values check speed (e.g. Color(3))

8f2f90c

Replace tuples with sets for faster lookups

ed43320

Used cls variable instead if multiple self.__class__

2c0372a

Merge remote-tracking branch 'origin/fast-enum' into fast-enum

cdc7041

# Conflicts: # Lib/enum.py

Bobronium requested a review from ethanfurman as a code owner December 20, 2019 03:01

the-knights-who-say-ni added the CLA not signed label Dec 20, 2019

bedevere-bot added the awaiting review label Dec 20, 2019

Bobronium and others added 2 commits December 20, 2019 15:00

Fix whitespace in types

48d75d0

📜🤖 Added by blurb_it.

ce84f10

ethanfurman self-assigned this Dec 20, 2019

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Dec 20, 2019

Bobronium added 7 commits December 21, 2019 06:08

Rename DynamicClassAttribute.__init__(name -> alias), add tests

0d1996f

DynamicClassAttribute.class_attr_name -> alias

2f6b977

Use _unique_member_map_ instead of _member_names_ to store unique enu…

1cedc4c

…m member

Use members dict instead of _member_names and _last_values on _EnumDict

8304b6f

Restore original formatting

4051208

Fix whitespaces

a90a4aa

Use dict instead of list to check values in _create_pseudo_member_

4436cbf

auvipy approved these changes Dec 22, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Dec 22, 2019

Deprecate getting values through _value_ and _name_

45be0c6

Just realized that _value_ and _name_ attrs remained independent from name and value and could be different. This commit fixes it.

Bobronium added a commit to Bobronium/fastenum that referenced this pull request Dec 22, 2019

Apply changes from python/cpython#17669

fc9837f

Run builtin python tests directly from python test module Refactor patch applying

Add 'name' and 'value' to Enum.__dir__

eabb7e3

ethanfurman reviewed Dec 22, 2019

View reviewed changes

Bobronium commented Dec 25, 2019

View reviewed changes

Bobronium requested a review from ethanfurman May 26, 2020 15:06

ethanfurman requested changes Apr 11, 2021

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Apr 11, 2021

97littleleaf11 mentioned this pull request Oct 4, 2021

Now OpExpr.op is a enum python/mypy#11263

Closed

ezio-melotti removed the CLA signed label Jul 13, 2022

MrMrRobat mannequin mentioned this pull request Aug 12, 2022

Increase Enum performance #83283

Closed

eendebakpt mentioned this pull request Aug 31, 2022

Triage the open "performance" issues on cpython repo faster-cpython/ideas#440

Closed

iritkatriel added the performance Performance or resource usage label Aug 31, 2022

ethanfurman closed this Dec 6, 2022

Bobronium deleted the fast-enum branch December 6, 2022 21:53

Bobronium restored the fast-enum branch December 6, 2022 21:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-39102: Increase Enum performance up to 10x times (3x average) #17669

bpo-39102: Increase Enum performance up to 10x times (3x average) #17669

Bobronium commented Dec 20, 2019 •

edited

Loading

the-knights-who-say-ni commented Dec 20, 2019

auvipy Dec 22, 2019

Bobronium Dec 22, 2019 •

edited

Loading

ethanfurman left a comment

Bobronium left a comment

Bobronium Dec 25, 2019

Bobronium Dec 25, 2019

Bobronium Dec 25, 2019

Bobronium commented May 26, 2020

TylerYep commented Jul 31, 2020

ethanfurman commented Jul 31, 2020

Bobronium commented Aug 1, 2020

belm0 commented Nov 23, 2020 •

edited

Loading

ethanfurman commented Nov 23, 2020

belm0 commented Nov 23, 2020

ethanfurman commented Nov 23, 2020

belm0 commented Nov 24, 2020

ethanfurman commented Nov 25, 2020

belm0 commented Dec 1, 2020 •

edited

Loading

ethanfurman left a comment

bedevere-bot commented Apr 11, 2021

belm0 commented Apr 11, 2021

ethanfurman commented Apr 12, 2021

Bobronium commented Apr 12, 2021

Bobronium commented Apr 12, 2021

ethanfurman commented Apr 12, 2021

bpo-39102: Increase Enum performance up to 10x times (3x average) #17669

bpo-39102: Increase Enum performance up to 10x times (3x average) #17669

Conversation

Bobronium commented Dec 20, 2019 • edited Loading

Increase Enum performance

Full benchmark result and source code

Changes

the-knights-who-say-ni commented Dec 20, 2019

CLA Missing

auvipy Dec 22, 2019

Choose a reason for hiding this comment

Bobronium Dec 22, 2019 • edited Loading

Choose a reason for hiding this comment

ethanfurman left a comment

Choose a reason for hiding this comment

Bobronium left a comment

Choose a reason for hiding this comment

Bobronium Dec 25, 2019

Choose a reason for hiding this comment

Bobronium Dec 25, 2019

Choose a reason for hiding this comment

Bobronium Dec 25, 2019

Choose a reason for hiding this comment

Bobronium commented May 26, 2020

TylerYep commented Jul 31, 2020

ethanfurman commented Jul 31, 2020

Bobronium commented Aug 1, 2020

belm0 commented Nov 23, 2020 • edited Loading

ethanfurman commented Nov 23, 2020

belm0 commented Nov 23, 2020

ethanfurman commented Nov 23, 2020

belm0 commented Nov 24, 2020

ethanfurman commented Nov 25, 2020

belm0 commented Dec 1, 2020 • edited Loading

ethanfurman left a comment

Choose a reason for hiding this comment

bedevere-bot commented Apr 11, 2021

belm0 commented Apr 11, 2021

ethanfurman commented Apr 12, 2021

Bobronium commented Apr 12, 2021

Bobronium commented Apr 12, 2021

ethanfurman commented Apr 12, 2021

Bobronium commented Dec 20, 2019 •

edited

Loading

Bobronium Dec 22, 2019 •

edited

Loading

belm0 commented Nov 23, 2020 •

edited

Loading

belm0 commented Dec 1, 2020 •

edited

Loading