Use faster method to get enum values

[mdboom]

See background discussion here.

It turns out that instead of

cdef cydriver.CUtensorMapInterleave cyinterleave = interleave.value

doing this:

cdef cydriver.CUtensorMapInterleave cyinterleave = int(interleave)

reduces the function call overhead of cuTensorMapEncodeTiled by about 25%. Not all API functions have that many enum arguments (5), so I wouldn't expect such a huge win across the board, but seems very worth doing nonetheless.

The reason this works is because IntEnum inherits from int, so there is a good fast path in C to grab its value quickly. enum.value has to do both a dictionary lookup for value and run the Python-written value property code.

[copy-pr-bot]

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.

[mdboom]

/ok to test

[kkraus14]

@mdboom great find. Is this something where the Cython generated cpp code could be optimized in the currently slow case to avoid all of those Python calls?

[mdboom]

@mdboom great find. Is this something where the Cython generated cpp code could be optimized in the currently slow case to avoid all of those Python calls?

Currently cuda-bindings' enums are based on the stdlib enum.IntEnum, which is implemented in Python and therefore we generate Python classes (not cdef classes) to define our enums. That means that all uses of them in Cython must go through Python, and additionally, stdlib enum is notoriously slow. They could be replaced with a more efficient C extension type. For the common case of just accessing the values in the enum, this would be quite easy to replace. However stdlib enum has a bunch of other features (like iterating over the values) that our users might be relying on that we would have to recreate (or methodically decide which to deprecate etc.). I think we should definitely do that at some point, as it's likely to be a big win (especially on import time).

[github-actions]

Backport failed for 12.9.x, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 12.9.x
git worktree add -d .worktree/backport-1543-to-12.9.x origin/12.9.x
cd .worktree/backport-1543-to-12.9.x
git switch --create backport-1543-to-12.9.x
git cherry-pick -x a7284ca5f37abd2ee6267bbf73a83ba63317686e

[github-actions]

Doc Preview CI
Preview removed because the pull request was closed or merged.

[jakirkham]

Cython does have a cpdef enum, which has a good mix of the C-level efficiency with the Python Enum behavior. Maybe worth considering

https://cython.readthedocs.io/en/latest/src/userguide/language_basics.html#structs-unions-enums

[leofang]

@mdboom it'd be nice to regenerate and backport this PR to 12.9.x.

@jakirkham Thanks for reminder. We avoided cpdef enum because of the namespace pollution, but it seems fixed since Cython 3.1.0. Would be a good idea to revisit this approach indeed. We've spent too much time hunting down Python Enum/IntEnum related perf issues (last occurrence: #439 (comment)).

[mdboom]

Cython does have a cpdef enum, which has a good mix of the C-level efficiency with the Python Enum behavior. Maybe worth considering

https://cython.readthedocs.io/en/latest/src/userguide/language_basics.html#structs-unions-enums

Unfortunately, cpdef enum is just syntactic sugar for the Python stdlib enum.Enum, and generating a C-level enum at the same time. We already have a C-level enum in our cy* layer, and the Python side is where all of the performance issues are, which Cython cpdef enum unfortunately doesn't address. Measurement of a prototype shows this actually regresses import times.

More info here: #1557

[jakirkham]

Thanks Michael and Leo! 🙏

Michael have you shared these issues with the Cython team?