Skip to content

gh-90213: Speed up right shifts of negative integers - #30277

Merged
markshannon merged 15 commits into
python:mainfrom
mdickinson:faster-rshift-of-negative
May 2, 2022
Merged

gh-90213: Speed up right shifts of negative integers#30277
markshannon merged 15 commits into
python:mainfrom
mdickinson:faster-rshift-of-negative

Conversation

@mdickinson

@mdickinsonmdickinson commented Dec 27, 2021

Copy link
Copy Markdown
Member

Taking some ideas from the work of @xuxinhang in #30044, this PR makes right shift of negative integers more efficient, by avoiding the need for temporary intermediate results.

Closes#90213

https://bugs.python.org/issue46055

@mdickinson

Copy link
Copy Markdown
MemberAuthor

One example timing: evaluating -7**100 >> 123 is approximately twice as fast on this branch as on main:

lovelace:cpython mdickinson$ git rev-parse main
3581c7abbe15bad6ae08fc38887e5948f8f39e08
lovelace:cpython mdickinson$ git rev-parse faster-rshift-of-negative
9bf9dcdd3342e9e770de23439065d46e07401a5c
lovelace:cpython mdickinson$ git checkout -q faster-rshift-of-negative && make >/dev/null 2>&1 && ./python.exe -m timeit -s "a = -7**100; b=123" "a>>b"
5000000 loops, best of 5: 44.8 nsec per loop
lovelace:cpython mdickinson$ git checkout -q main && make >/dev/null 2>&1 && ./python.exe -m timeit -s "a = -7**100; b=123" "a>>b"
5000000 loops, best of 5: 95 nsec per loop

@mdickinson

Copy link
Copy Markdown
MemberAuthor

Smaller and larger examples also benefit on my machine: -1234567890 >> 8 is approximately 1.9 times faster; -7**100000 >> 12345 is approximately 2.8 times faster:

lovelace:cpython mdickinson$ git checkout -q main && make >/dev/null 2>&1 && ./python.exe -m timeit -s "a = -1234567890; b=8" "a>>b"
5000000 loops, best of 5: 81.5 nsec per loop
lovelace:cpython mdickinson$ git checkout -q faster-rshift-of-negative && make >/dev/null 2>&1 && ./python.exe -m timeit -s "a = -1234567890; b=8" "a>>b"
5000000 loops, best of 5: 42.6 nsec per loop
lovelace:cpython mdickinson$ git checkout -q main && make >/dev/null 2>&1 && ./python.exe -m timeit -s "a = -7**100000; b=12345" "a>>b"
20000 loops, best of 5: 19.3 usec per loop
lovelace:cpython mdickinson$ git checkout -q faster-rshift-of-negative && make >/dev/null 2>&1 && ./python.exe -m timeit -s "a = -7**100000; b=12345" "a>>b"
50000 loops, best of 5: 6.82 usec per loop

Comment threadObjects/longobject.c Outdated
Comment threadObjects/longobject.c Outdated
Comment threadObjects/longobject.c Outdated
@mdickinson

Copy link
Copy Markdown
MemberAuthor

I've reworked to remove the duplication between the positive and negative branches, and to move the shift==0 check earlier.

@mdickinsonmdickinson added performance Performance or resource usage and removed skip news labels Dec 28, 2021
Comment threadObjects/longobject.c Outdated
Comment threadMisc/NEWS.d/next/Core and Builtins/2021-12-24-20-21-45.bpo-46055.R0QMVQ.rst Outdated
@mdickinsonmdickinson added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 9, 2022
@bedevere-bot

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @mdickinson for commit b3a1a8f 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 9, 2022
@mdickinsonmdickinson added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 9, 2022
@bedevere-bot

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @mdickinson for commit fd4ce81 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 9, 2022
@github-actions

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actionsgithub-actionsBot added the stale Stale PR or inactive for long period of time. label Feb 9, 2022
@markshannonmarkshannon self-assigned this Feb 28, 2022

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

I'm not sure that I understand 100%, but I think it is correct and it does pass the tests.

A few of the variables could have their scope narrowed.

Comment threadObjects/longobject.c
Comment threadObjects/longobject.c Outdated
Comment threadObjects/longobject.c Outdated
Comment threadObjects/longobject.c Outdated
Comment threadObjects/longobject.c Outdated
Comment threadObjects/longobject.c Outdated
Comment threadObjects/longobject.c Outdated
@mdickinsonmdickinson removed the stale Stale PR or inactive for long period of time. label Apr 12, 2022
@mdickinsonmdickinson changed the title bpo-46055: Speed up right shifts of negative integersgh-90213: Speed up right shifts of negative integersApr 12, 2022
@mdickinson

Copy link
Copy Markdown
MemberAuthor

@markshannon Thanks for the review! I've addressed your comments, and I think this should be ready for re-review.

@mdickinsonmdickinson added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 12, 2022
@bedevere-bot

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @mdickinson for commit c5e3f81 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 12, 2022
@markshannon

Copy link
Copy Markdown
Member

AFAICT this looks good, and all the buildbots are happy.

@markshannon
markshannon merged commit 0ed91a2 into python:mainMay 2, 2022
@xuxinhangxuxinhangmannequin mentioned this pull request Apr 10, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performancePerformance or resource usage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Speed up binary shifting operators

5 participants

@mdickinson@bedevere-bot@markshannon@serhiy-storchaka@the-knights-who-say-ni