← 返回首页
gh-99761: add invalid_index macro by eendebakpt · Pull Request #99762 · python/cpython · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Include my email address so I can be contacted
Cancel Submit feedback

Saved searches

Use saved searches to filter your results more quickly

Cancel Create saved search
Appearance settings
Resetting focus

gh-99761: add invalid_index macro#99762

Closed
eendebakpt wants to merge 52 commits into
python:mainfrom
eendebakpt:list_tuple
Closed

gh-99761: add invalid_index macro#99762
eendebakpt wants to merge 52 commits into
python:mainfrom
eendebakpt:list_tuple

Conversation

eendebakpt commented Nov 24, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

In listobject.c there is an optimization 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 and bytesobject.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.

Notes:

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

bedevere-bot mentioned this pull request Nov 24, 2022
Closed
eendebakpt marked this pull request as draft November 24, 2022 22:12

sweeneyde commented Nov 24, 2022

Copy link
Copy Markdown
Member

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...'. See https://devguide.python.org/developer-workflow/c-api/ and https://peps.python.org/pep-0007/#naming-conventions .

cc @vstinner

eendebakpt commented Nov 25, 2022

Copy link
Copy Markdown
Contributor Author

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...'. See https://devguide.python.org/developer-workflow/c-api/ and https://peps.python.org/pep-0007/#naming-conventions .

cc @vstinner

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

rhettinger commented Nov 26, 2022

Copy link
Copy Markdown
Contributor

  • Keep the code as valid_index rather than invalid_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 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.

  • 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 commented Nov 26, 2022

Copy link
Copy Markdown
Contributor Author
  • Keep the code as valid_index rather than invalid_index so that we don't even up with double negatives.

Everywhere in the code the usage is !valid_index, which is why I preferred invalid_index. I renamed to valid_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 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.

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.

The valid_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

vstinner commented Nov 28, 2022

Copy link
Copy Markdown
Member

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

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

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.

Comment thread Include/internal/pycore_abstract.h Outdated Show resolved Hide resolved
39 hidden items Load more…
arhadthedev reviewed Feb 20, 2023
View reviewed changes
Comment thread Include/internal/pycore_abstract.h Show resolved Hide resolved

eendebakpt commented Mar 6, 2023

Copy link
Copy Markdown
Contributor Author

The overall change LGTM, but please address my two remaining change requests.

@vstinner Could you have a look at the PR again?

arhadthedev added interpreter-core (Objects, Python, Grammar, and Parser dirs) extension-modules C modules in the Modules dir skip news labels Apr 5, 2023
eendebakpt requested a review from arhadthedev April 28, 2023 19:07
arhadthedev approved these changes May 20, 2023
View reviewed changes

arhadthedev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

All earlier reviews seem to be addressed.

eendebakpt mentioned this pull request Jun 8, 2024
Closed

serhiy-storchaka commented Jun 8, 2024

Copy link
Copy Markdown
Member

This idea was already discussed and rejected. See #72583.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

arhadthedev arhadthedev approved these changes
vstinner Awaiting requested review from vstinner

Assignees

No one assigned

Labels

awaiting core review extension-modules C modules in the Modules dir interpreter-core (Objects, Python, Grammar, and Parser dirs) skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Footer

© 2026 GitHub, Inc.