Faster enum implementation (Pure Python version) #1581
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test |
|
|
/ok to test |
3 similar comments
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test |
leofang
added
enhancement
| It supports the most important subset of the IntEnum API. See `test_enum` in | ||
| `cuda_bindings/tests/test_basics.py` for details. |
There was a problem hiding this comment.
Based on this comment I assume this means that we're only supporting a subset of an IntEnum and given we're returning these that would probably constitute an API break?
I.e. because we were previously returning an IntEnum as error codes, if someone has code that does something like isinstance(value, enum.IntEnum), that would then start returning False?
There was a problem hiding this comment.
Yes. But moving away from Enum/IntEnum is our only route to gain performance on the error handling side. I think this ducktyping is good enough and the break due to type checking seems to be theoretical. Users most likely would just write isinstance(err, CUresult) (which is nonbreaking) instead of isinstance(err, Enum).
There was a problem hiding this comment.
Yeah -- I'm probably being overly cautious with the word subset here. I carefully looked at everything IntEnum exposes and replicated it here. The test even tests stdlib and our implementation using the same test code. You are right that the biggest visible change is the isinstance(value, enum.IntEnum) thing, but I agree with @leofang, that seems unlikely.
In any event, I'll add a CHANGELOG entry here.
There was a problem hiding this comment.
If we inherited from enum.IntEnum instead of int would that nullify some of the performance gains?
Could we use __instancecheck__ to make isinstance(enum.IntEnum) and isinstance(enum.Enum) work as they did previously?
Either of these would likely limit the breakage to if someone was using anti-patterns like using type(...) or something like that.
There was a problem hiding this comment.
If we inherited from enum.IntEnum instead of int would that nullify some of the performance gains?
Yeah, unfortunately you can't bring in the value type (IntEnum) without bringing in the metaclass and all of its startup cost:
TypeError: metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases
Could we use instancecheck to make isinstance(enum.IntEnum) and isinstance(enum.Enum) work as they did previously?
Unfortunately not. __instancecheck__ is a method on the type/metaclass that you are testing for. In other words, to make isinstance(FastEnum.VALUE, enum.IntEnum) work, we would have to monkeypatch an __instancecheck__ to enum.IntEnum.
This GitHub code search with a regex (for isinstance\(.*, IntEnum\) in a file that also contains the word cuda throws up a few possible problematic things. I guess we need to decide whether the performance matters enough that it's worth the pain to help our users fix those things in a few places.
There was a problem hiding this comment.
Monkeypatching IntEnum's __instancecheck__ does work, with the loss of some performance:
+------------------------------+-------------------+-----------------------+
| isinstance(enum, IntEnum) | 48.1 ns | 80.6 ns: 1.68x slower |
+------------------------------+-------------------+-----------------------+
If performance matters to our users, we can suggest updating their code to isinstance(enum, FastEnum), which has no penalty from the monkeypatching.
Maybe this is an ok solution for backward compatibility.
There was a problem hiding this comment.
@leofang, @kkraus14: So I think we just need to do decide:
- We merge this as-is where
isinstance(enum, IntEnum)returnsFalse. - We include a monkeypatch to make
isinstance(enum, IntEnum)work. This makes that isinstance check about 68% slower (but that check is exclusive to frameworks doing things in general, not really in user code), but we wouldn't break dependent libraries. As with all monkeypatches, unintended consequences are a little hard to reason about. I /think/ this is safe, but I wouldn't bet my life on it. - We don't do this whole thing right now. (And, as a reminder to myself, to revert the generator changes if that's what we decide).
This is what (2) looks like:
def __instancecheck__(cls, instance) -> bool:
if isinstance(instance, FastEnum):
return True
return issubclass(type(instance), IntEnum)
type(IntEnum).__instancecheck__ = __instancecheck__There was a problem hiding this comment.
I vote for 1 and if there is any bug report we do 2 in a patch release.
There was a problem hiding this comment.
+1. I'd vote for 1 and then fallback to 2 if required based on user feedback.
There was a problem hiding this comment.
I'm good with that, lets ship it
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
4 participants
This adds an implementation of integer enums that is significantly faster than the one in the stdlib (
enum.IntEnum).There was an alternate implementation of this that used a handwritten C extension, but the performance benefit was minor. This is both much faster than the stdlib, but also small and easy-to-maintain.
Additionally, this lets us assign docstrings to the enumeration values, which is a feature the stdlib enum doesn't have. Then all of the generated code has been updated to include docstrings, auto-derived from the headers.