NotificationsYou must be signed in to change notification settings
Fork33.3k
Star69.8k

gh-99761: add invalid_index macro#99762

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

eendebakpt wants to merge52 commits intopython:mainfromeendebakpt:list_tuple

Closed

gh-99761: add invalid_index macro#99762

eendebakpt wants to merge52 commits intopython:mainfromeendebakpt:list_tuple

Conversation

Copy link

Contributor

eendebakpt commentedNov 24, 2022•
edited
Loading

Inlistobject.c there is anoptimization to check whether an index is valid (e.g.0 <= index < N) using a single comparison. The cast-to-unsigned optimization is used in current main in 3 locations:tupleobject.c,_collectionsmodule.c andbytesobject.c.

It is a micro optimization, but the code generated is different than the plain i < 0 || i >= Py_SIZE(a). This PR tries to:

i) apply the optimization to more locations
ii) make the code more consistent

By replacing index checks with a single method_Py_is_valid_index that includes the optimization we have consistency in the code and have the optimized check for all index checks.

Issue:Consistent use of valid index check #99761

Notes:

The new method is private and is static inline (https://peps.python.org/pep-0670/)
Inbytecodes.c the optimized expression is used in three places. E.g.

DEOPT_IF(((size_t)signed_magnitude) > 1, BINARY_SUBSCR);

With the_Py_is_valid_index method it looks like

DEOPT_IF(!_Py_is_valid_index(signed_magnitude, 2), BINARY_SUBSCR);

which is in my opinion not very readable. For these cases a separate PR was created:#100064

add invalid_index

0d1b08a

eendebakpt requested review frommarkshannon andrhettinger ascode owners

November 24, 2022 22:11

bedevere-bot mentioned this pull request

Nov 24, 2022

Consistent use of valid index check#99761

Closed

bedevere-bot added the awaiting review label

Nov 24, 2022

eendebakpt marked this pull request as draft

November 24, 2022 22:12

Copy link

Member

sweeneyde commentedNov 24, 2022

I think this is generally a good idea, but I'm not sure this should go in the public API. If it does, its name should probably begin with 'Py...'. Seehttps://devguide.python.org/developer-workflow/c-api/ andhttps://peps.python.org/pep-0007/#naming-conventions .

cc@vstinner

eendebakptand others added5 commits

November 25, 2022 09:55

replace valid_index

be6143f

📜🤖 Added by blurb_it.

bcaec88

make regen-all

6dd6fec

Merge branch 'list_tuple' of github.com:eendebakpt/cpython into list_…

7a35197

…tuple

Rename 2022-11-25-09-34-57.gh-issue-9761.sRqH37.rst to 2022-11-25-09-…

1e4448f

…34-57.gh-issue-99761.sRqH37.rst

Copy link

ContributorAuthor

eendebakpt commentedNov 25, 2022

I think this is generally a good idea, but I'm not sure this should go in the public API. If it does, its name should probably begin with 'Py...'. Seehttps://devguide.python.org/developer-workflow/c-api/ andhttps://peps.python.org/pep-0007/#naming-conventions .
cc@vstinner

Agreed. What would be a good location for the private version? One of theInclude/internal/python_xxxx files?

eendebakpt force-pushed thelist_tuple branch from37a83e2 tod85a883Compare

November 25, 2022 19:14

add method for int check

1d58ef0

eendebakpt force-pushed thelist_tuple branch fromd85a883 to1d58ef0Compare

November 25, 2022 19:15

Copy link

Contributor

rhettinger commentedNov 26, 2022

Keep the code asvalid_index rather thaninvalid_index so that we don't even up with double negatives.
Please leave the collections.deque code alone. The author (me) aspires to keep this code not tightly coupled to the rest of the C API. The code is currently clean an doesn't need to be changed.
Are youcertain that inline functions always get inlined? We know that macros doalways get inlined. Thevalid_index macro currently gets used in performance critical code.
As Dennis says, this should not be part of the public API. It is a performance trick that happens to currently be useful. As compilers get better, this would be done automatically and we would prefer the simple inline code rather than the extra layer of abstraction.

eendebakpt added8 commits

November 26, 2022 22:11

change invalid_index to valid_index

76ecde6

change invalid_index to valid_index

c356130

revert changes to _collectionsmodule.c on request of the author

329d6ed

make valid_index private

bbdb3b5

change static inline to macro

45d4abf

typo

e78d129

unto changes to _collectionsmodule.c

5cc1dfc

Merge branch 'main' into list_tuple

a112a05

Copy link

ContributorAuthor

eendebakpt commentedNov 26, 2022

Keep the code asvalid_index rather thaninvalid_index so that we don't even up with double negatives.

Everywhere in the code the usage is!valid_index, which is why I preferredinvalid_index. I renamed tovalid_index again.

Please leave the collections.deque code alone. The author (me) aspires to keep this code not tightly coupled to the rest of
the C API. The code is currently clean an doesn't need to be changed.

Done.

Are youcertain that inline functions always get inlined? We know that macros doalways get inlined. Thevalid_index macro currently gets used in performance critical code.

With inline functions I think we can never be certain the code gets inlined for all compilers and settings. I changed the static inline to a macro.

As Dennis says, this should not be part of the public API. It is a performance trick that happens to currently be useful. As compilers get better, this would be done automatically and we would prefer the simple inline code rather than the extra layer of abstraction.

Thevalid_index is now private (part of Includes/internal/pycore_abstract.h). If there is a more suitable location we can move it.

@rhettinger

eendebakpt marked this pull request as ready for review

November 26, 2022 22:12

Copy link

Member

vstinner commentedNov 28, 2022

You should not use macro but a static inline functions, see:

@rhettinger:

Are you certain that inline functions always get inlined? We know that macros do always get inlined. The valid_index macro currently gets used in performance critical code.

See PEP 670 which answers to that and we approved by the Steering Council. A static inline function must be used.

vstinner reviewed

Nov 28, 2022

View reviewed changes

Copy link

Member

vstinner left a comment

Choose a reason for hiding this comment

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

I'm not very excited by a function only used for a micro-optimization, I'm fine with(i < 0 || i >= Py_SIZE(self)) test. But I'm not against this change neither.