GeezerVison is showing github.com — gh-99761: add invalid_index macro by eendebakpt · Pull Request #99762 · python/cpython
Conversation
eendebakpt
commented
Nov 24, 2022
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
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
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
…34-57.gh-issue-99761.sRqH37.rst
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
-
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
- 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
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
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.
|
erlend-aasland reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this 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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
The overall change LGTM, but please address my two remaining change requests.
@vstinner Could you have a look at the PR again?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All earlier reviews seem to be addressed.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
This idea was already discussed and rejected. See #72583.
|
eendebakpt reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
vstinner
Awaiting requested review from vstinner
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|