GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-46055: Speed up binary shifting operators by xuxinhang · Pull Request #30044 · python/cpython

Conversation

@xuxinhang

@xuxinhang xuxinhang commented Dec 11, 2021 •
edited by merwok
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Inspired by https://bugs.python.org/issue44946, I found there were no special shortcuts for shifting operation applied to "medium value" long object. So I modified CPython's VM to accelerate my python project where there is plenty of binary shifting operation. I guess somebody else might also need it.

Thanks.

https://bugs.python.org/issue46055

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@xuxinhang
Perf: Accelerate binary shift op.
7b11d7d
@the-knights-who-say-ni

the-knights-who-say-ni commented Dec 11, 2021

Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@xuxinhang

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@the-knights-who-say-ni the-knights-who-say-ni added the CLA not signed label Dec 11, 2021
@bedevere-bot bedevere-bot added the awaiting review label Dec 11, 2021
@sweeneyde

sweeneyde commented Dec 11, 2021

Copy link
Copy Markdown
Member

Hi @xuxinhang, it would be great if you could open an issue on bugs.python.org to associate with this PR, and change the PR title accordingly.

Also, please make sure to get the CLA signed. Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

xuxinhang added 2 commits December 12, 2021 12:59
@xuxinhang
Fix: Ubuntu gcc segment fault.
403652a
@xuxinhang
Let PyLong_New accept a negtive size.
dcd2796
@xuxinhang xuxinhang changed the title Speed up binary shifting operators. bpo-46055: Speed up binary shifting operators Dec 12, 2021
@xuxinhang

xuxinhang commented Dec 12, 2021

Copy link
Copy Markdown
Contributor Author

Sorry, I have create a new issue in the bug tracker and I'm waiting for CLA getting verified. Besides, the building in Ubuntu doesn't complain segment fault now.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sweeneyde
sweeneyde reviewed Dec 12, 2021
View reviewed changes

@sweeneyde sweeneyde left a comment

Copy link
Copy Markdown
Member

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 didn't do a complete review, but here are a few comments.

It would also be nice to see some benchmarks for the change.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Objects/longobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/longobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/longobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/longobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/longobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

sweeneyde
sweeneyde reviewed Dec 12, 2021
View reviewed changes
Comment thread Objects/longobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

xuxinhang added 2 commits December 12, 2021 20:04
@xuxinhang
Fix: Use Py_ARITHMETIC_RIGHT_SHIFT to avoid the undefined padding bah…
25bbf4d
…avior of C right shift operator.
@xuxinhang
Style: PEP7: add braces.
b2ef93d
@xuxinhang

xuxinhang commented Dec 12, 2021

Copy link
Copy Markdown
Contributor Author

BENCHMARKS

I use timeit to measure time costs and any other operators or calls are excluded. For each testcase, the former is dcd2796 and the latter is this PR's base 036bbb1.

64-bit Release building. Run in Windows 10 1709 (64-bit)

python -m timeit " i = 1; i <<= 3; i >>= 3"  # small value (cost down by 36%)
5000000 loops, best of 5: 92.7 nsec per loop
2000000 loops, best of 5: 145 nsec per loop

python -m timeit " i = 1; i <<= 10; i >>= 10"  # medium value (-25%)
2000000 loops, best of 5: 114 nsec per loop
2000000 loops, best of 5: 151 nsec per loop

python -m timeit " i = 1; i <<= 100; i >>= 100"  # big value  (-12%)
1000000 loops, best of 5: 209 nsec per loop
1000000 loops, best of 5: 238 nsec per loop
sweeneyde reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sweeneyde
sweeneyde reviewed Dec 13, 2021
View reviewed changes
Comment thread Objects/longobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@the-knights-who-say-ni the-knights-who-say-ni added CLA signed and removed CLA not signed labels Dec 13, 2021
@xuxinhang
xuxinhang force-pushed the feat/binary_shift_optimization branch 4 times, most recently from 52aebcb to b2ef93d Compare December 13, 2021 15:56
@xuxinhang
Fix: Bypass undefined shift operator behavior
7d2203f
@sweeneyde
sweeneyde requested a review from mdickinson December 22, 2021 06:37
sweeneyde
sweeneyde reviewed Dec 22, 2021
View reviewed changes
Comment thread Objects/longobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

sweeneyde
sweeneyde reviewed Dec 22, 2021
View reviewed changes
Comment thread Objects/longobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

mdickinson
mdickinson reviewed Dec 22, 2021
View reviewed changes

@mdickinson mdickinson left a comment

Copy link
Copy Markdown
Member

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.

There are a few changes in this PR that aren't directly related to the stated goal of speeding up the shift operators. Please could you revert the unrelated changes, so that we can focus on the shift operations? That would make the PR easier to review and evaluate.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
mdickinson
mdickinson reviewed Dec 22, 2021
View reviewed changes
Comment thread Objects/longobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

mdickinson
mdickinson reviewed Dec 22, 2021
View reviewed changes
Comment thread Objects/longobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

mdickinson
mdickinson reviewed Dec 22, 2021
View reviewed changes
Comment thread Objects/longobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

mdickinson
mdickinson reviewed Dec 22, 2021
View reviewed changes
Comment thread Objects/longobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@xuxinhang
Docs: A couple more things.
c359521
@xuxinhang

xuxinhang commented Dec 24, 2021

Copy link
Copy Markdown
Contributor Author

Thanks.
——
I have made the requested changes; please review again

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Dec 24, 2021
@bedevere-bot

bedevere-bot commented Dec 24, 2021

Copy link
Copy Markdown

Thanks for making the requested changes!

@mdickinson: please review the changes made to this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot
bedevere-bot requested a review from mdickinson December 24, 2021 12:33
@mdickinson

mdickinson commented Dec 24, 2021

Copy link
Copy Markdown
Member

@xuxinhang: Almost there. Please could you undo the changes to the medium_value function?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@xuxinhang
Revert: medium_value
241da4c
@xuxinhang
xuxinhang force-pushed the feat/binary_shift_optimization branch from 4482861 to 241da4c Compare December 24, 2021 13:22
sweeneyde
sweeneyde approved these changes Dec 25, 2021
View reviewed changes
mdickinson
mdickinson reviewed Dec 27, 2021
View reviewed changes
Comment thread Misc/NEWS.d/next/Core and Builtins/2021-12-24-20-21-45.bpo-46055.R0QMVQ.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@mdickinson
Add attribution; fix typo
9ed09aa
mdickinson
mdickinson approved these changes Dec 27, 2021
View reviewed changes

@mdickinson mdickinson left a comment

Copy link
Copy Markdown
Member

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.

LGTM! Merging when the CI completes. Thank you!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels Dec 27, 2021
@mdickinson
Merge branch 'main' into feat/binary_shift_optimization
c24ad0a
@mdickinson mdickinson added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 27, 2021
@bedevere-bot

bedevere-bot commented Dec 27, 2021

Copy link
Copy Markdown

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

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 27, 2021
@mdickinson

mdickinson commented Dec 27, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Some sample timings on my machine. "main" is at commit b8de8b7. "here" is this branch at commit 9ed09aa, after a merge with main. Timings performed on an Intel MacBook Pro, macOS 10.14.6, with ./python.exe -m timeit -s "i, s = 1, 3" "i << s" or ./python.exe -m timeit -s "i, s = 1, 3" "i >> s", with suitable values of i and s in the setup.

For 1<<3:
main: 5000000 loops, best of 5: 38.2 nsec per loop
here: 10000000 loops, best of 5: 24.7 nsec per loop
55% faster

For 1234567 >> 13:
main: 5000000 loops, best of 5: 38.2 nsec per loop
here: 10000000 loops, best of 5: 24.7 nsec per loop
55% faster

(No, those numbers aren't copy and paste errors - I really did get the exact same timings.)

For 1234567 >> 123:
main: 10000000 loops, best of 5: 23.3 nsec per loop
here: 10000000 loops, best of 5: 25.3 nsec per loop
8% slower (though this seems to be within random variation)

For -1234567 >> 13:
main: 5000000 loops, best of 5: 67.5 nsec per loop
here: 10000000 loops, best of 5: 36.2 nsec per loop
86% faster

For -1234567 >> 123:
main: 5000000 loops, best of 5: 39.5 nsec per loop
here: 10000000 loops, best of 5: 25.1 nsec per loop
57% faster

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mdickinson

mdickinson commented Dec 27, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

The Tests / Windows (x64) (pull_request) check seems to be stuck at the build step (1h57m and counting). Closing and re-opening to re-trigger the build.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mdickinson mdickinson closed this Dec 27, 2021
@mdickinson mdickinson reopened this Dec 27, 2021
@serhiy-storchaka

serhiy-storchaka commented Dec 27, 2021

Copy link
Copy Markdown
Member

I suggest to merge #30243 first and compare this PR with a new base.

mdickinson reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mdickinson
Merge remote-tracking branch 'upstream/main' into feat/binary_shift_o…
59672cf
…ptimization
@mdickinson

mdickinson commented Dec 27, 2021

Copy link
Copy Markdown
Member

Some new sample timings, now that #30243 has been merged into master, using the same machine and methodology as above. It's interesting that 1234567 >> 13 gets a significant speedup on my machine, but 12345678 >> 13 doesn't; looks like in the second case, the time is dominated by the time to construct a new PyLong for the result. (In the first case, the result is small.)

Comparing commits:
here: 59672cf
main: 360fedc

1 << 3: ~43% faster

  • here: 10000000 loops, best of 5: 26.7 nsec per loop
  • main: 10000000 loops, best of 5: 38.1 nsec per loop

1234567 >> 13: ~36% faster

  • here: 10000000 loops, best of 5: 26.6 nsec per loop
  • main: 10000000 loops, best of 5: 36.2 nsec per loop

12345678 >> 13: ~6% faster

  • here: 10000000 loops, best of 5: 34.3 nsec per loop
  • main: 10000000 loops, best of 5: 36.4 nsec per loop

-1234567 >> 13: ~77% faster

  • here: 5000000 loops, best of 5: 38.8 nsec per loop
  • main: 5000000 loops, best of 5: 68.6 nsec per loop
xuxinhang and gpshead reacted with hooray emoji
All reactions
  • 🎉 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mdickinson
mdickinson merged commit 3581c7a into python:main Dec 27, 2021
@bedevere-bot bedevere-bot removed the awaiting merge label Dec 27, 2021
@mdickinson mdickinson mentioned this pull request Dec 27, 2021
gh-90213: Speed up right shifts of negative integers #30277
Merged
@xuxinhang xuxinhang mannequin mentioned this pull request Apr 10, 2022
Speed up binary shifting operators #90213
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments
@sweeneyde sweeneyde sweeneyde approved these changes
+2 more reviewers
@tim-one tim-one tim-one left review comments
@mdickinson mdickinson mdickinson approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

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.

7 participants

@xuxinhang @the-knights-who-say-ni @sweeneyde @bedevere-bot @mdickinson @serhiy-storchaka @tim-one
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.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.