Skip to content

PyTuple: use fast macros on !abi3#1653

Merged
davidhewitt merged 1 commit intomainfrom
pytuple_fast
Jun 5, 2021
Merged

PyTuple: use fast macros on !abi3#1653
davidhewitt merged 1 commit intomainfrom
pytuple_fast

Conversation

@birkenfeld
Copy link
Member

They were already defined, but not used so far. For abi3 they now forward
to the C functions.

Question: should these be defined at all for abi3, or should I add the conditional code in the PyTuple impl?

@davidhewitt
Copy link
Member

It looks like these macros are only defined in https://github.com/python/cpython/blob/main/Include/cpython/tupleobject.h, i.e. for the unlimited abi.

So I'd vote for doing the same for our ffi module - only define these macros for the unlimited abi and write the conditional code inside our tuple code.

If there's lots of places in the code where this happens, can always add tiny #[inline] pub(crate) fn pytuple_set_item helpers which are defined differently for each #[cfg] and call to the relevant ffi function.

They were already defined, but not used so far.
@birkenfeld
Copy link
Member Author

So far, these were each only used in one place, so I went and added the conditional there.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice one!

FWIW, I get ~2% speedup with tuple_get_item benchmark on this branch, and ~8% speedup on a tuple_new benchmark I'll push in a minute.

@davidhewitt davidhewitt merged commit 3292ccf into main Jun 5, 2021
@birkenfeld birkenfeld deleted the pytuple_fast branch June 5, 2021 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants