Skip to content

gh-120176: Reduce number of compares in index range checks - #120181

Closed
lgeiger wants to merge 1 commit into
python:mainfrom
lgeiger:faster-index-checks
Closed

gh-120176: Reduce number of compares in index range checks#120181
lgeiger wants to merge 1 commit into
python:mainfrom
lgeiger:faster-index-checks

Conversation

@lgeiger

@lgeigerlgeiger commented Jun 6, 2024

Copy link
Copy Markdown
Contributor

This PR is a followup to #9784 (discussed in #72583 and #67741) and extracts the helper function into internal/pycore_abstract.h and uses it where applicable to reduce the number of comparisons in index range checks.

See https://godbolt.org/z/jM8oexT1d for an assembly comparison.

I feel like this change doesn't reduce readability but let me know if such micro optimisation does not justify the code churn.

@ghost

ghost commented Jun 6, 2024

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@methane

Copy link
Copy Markdown
Member

No news entry is required because this change is not visible to users.
I assume performance gain is not large enough to have news entry.

@methanemethane 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.

LGTM.
I will wait a week before merging this PR to ensure that other committers have the opportunity to review it.

@eendebakpt

Copy link
Copy Markdown
Contributor

@lgeiger@methane This pr is nearly identical to #99762. There are some more review comments there

@serhiy-storchaka

Copy link
Copy Markdown
Member

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

@lgeiger

Copy link
Copy Markdown
ContributorAuthor

Ah sorry, didn't see #99762

Sorry about that!

@lgeiger
lgeiger deleted the faster-index-checks branch June 8, 2024 20:46
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@lgeiger@methane@eendebakpt@serhiy-storchaka