GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||
GeezerVison is showing github.com — bpo-46055: Speed up binary shifting operators by xuxinhang · Pull Request #30044 · python/cpython Conversation
|
|
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! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
…avior of C right shift operator.
BENCHMARKSI use 64-bit Release building. Run in Windows 10 1709 (64-bit) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
52aebcb to
b2ef93d
Compare
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Thanks. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks for making the requested changes! @mdickinson: please review the changes made to this pull request. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@xuxinhang: Almost there. Please could you undo the changes to the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
4482861 to
241da4c
Compare
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
|
🤖 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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
The |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I suggest to merge #30243 first and compare this PR with a new base. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.