Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
bpo-46055: Speed up binary shifting operators#30044
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpo-46055: Speed up binary shifting operators #30044
Uh oh!
There was an error while loading. Please reload this page.
Conversation
xuxinhang commented Dec 11, 2021 • edited by merwok
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by merwok
Uh oh!
There was an error while loading. Please reload this page.
the-knights-who-say-ni commented Dec 11, 2021
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 MissingOur records indicate the following people have not signed the CLA: 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 You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
sweeneyde commented Dec 11, 2021
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! |
xuxinhang commented Dec 12, 2021
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. |
sweeneyde left a comment
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.
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.
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.
…avior of C right shift operator.
xuxinhang commented Dec 12, 2021
BENCHMARKSI use 64-bit Release building. Run in Windows 10 1709 (64-bit) |
Uh oh!
There was an error while loading. Please reload this page.
52aebcb to b2ef93dCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
mdickinson left a comment
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.
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.
Uh oh!
There was an error while loading. Please reload this page.
xuxinhang commented Dec 24, 2021
Thanks. |
bedevere-bot commented Dec 24, 2021
Thanks for making the requested changes! @mdickinson: please review the changes made to this pull request. |
mdickinson commented Dec 24, 2021
@xuxinhang: Almost there. Please could you undo the changes to the |
4482861 to 241da4cCompareMisc/NEWS.d/next/Core and Builtins/2021-12-24-20-21-45.bpo-46055.R0QMVQ.rst Outdated Show resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
mdickinson left a comment
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!
bedevere-bot commented Dec 27, 2021
🤖 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. |
mdickinson commented Dec 27, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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 For For (No, those numbers aren't copy and paste errors - I really did get the exact same timings.) For For For |
mdickinson commented Dec 27, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
The |
serhiy-storchaka commented Dec 27, 2021
I suggest to merge #30243 first and compare this PR with a new base. |
mdickinson commented Dec 27, 2021
Some new sample timings, now that #30243 has been merged into master, using the same machine and methodology as above. It's interesting that Comparing commits:
|
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