Skip to content
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-31031: Unify duplicate bits_in_digit and bit_length #2866

Merged
merged 1 commit into from Jan 16, 2020

Conversation

@niklasf
Copy link
Contributor

niklasf commented Jul 25, 2017

Copy link

auvipy left a comment

check the build errors plz

@niklasf niklasf force-pushed the niklasf:bpo-31031-consolidate-bit-length branch from ccdbe59 to 900f7a8 Jun 2, 2019
Include/pymath.h Show resolved Hide resolved
* Equivalent to floor(lg(x))+1. Also equivalent to: bitwidth_of_type -
* count_leading_zero_bits(x)
*/
extern unsigned int _Py_bit_length(unsigned long d);

This comment has been minimized.

Copy link
@vstinner

vstinner Jun 4, 2019

Member

You should use PyAPI_FUNC(unsigned int) rather than extern.

By the way, _Py symbols should be moved to a new Include/cython/pymath.h header IMHO, and excluded from the stable API.

This comment has been minimized.

Copy link
@niklasf

niklasf Jun 5, 2019

Author Contributor

(1) Done.

(2) Sounds reasonable. Would you prefer to start this new file now (and move the other private functions later), or a seperate patch that moves all of the existing private functions including this new one?


unsigned int _Py_bit_length(unsigned long d) {
unsigned int d_bits = 0;
while (d >= 32) {

This comment has been minimized.

Copy link
@vstinner

vstinner Jun 4, 2019

Member

Can't we unroll manually the loop using SIZEOF_LONG? For example, 4 bytes long need 0 loop. 8 bytes long only require a single if(). There is no platform supported by Python with long larger than 8 bytes (64 bits).

This comment has been minimized.

Copy link
@mdickinson

mdickinson Jun 4, 2019

Member

A 4-byte long would need up to 6 iterations (and an 8-byte long up to 11): we're only removing 6 bits per iteration.

This comment has been minimized.

Copy link
@niklasf

niklasf Jun 5, 2019

Author Contributor

This can probably be optimized to work without branching (using the __clz intrinsic or equivalents). But I'd prefer to keep this patch as a pure refactoring, simply chosing the better of the existing implementations, for now.

This comment has been minimized.

Copy link
@vstinner

vstinner Jun 5, 2019

Member

Oh wait, I didn't notice that it's "only" 6 bits per iteration. I read 32 bits by mistake. You can ignore my comment.

This comment has been minimized.

Copy link
@vstinner

vstinner Jun 5, 2019

Member

Using clz was discussed in: https://bugs.python.org/issue29782

But it was rejected: https://bugs.python.org/issue29782#msg290973

Maybe the best we can do is to add a short comment to explain why we use a "naive" implementation (not really naive, it uses a precomputed table) linking to bpo-29782.

Would you mind to add such short comment?

By the way, int.bit_length() has a complexity of compleO(1) in practice, no? (I consider that this function has a complexity of O(1), especially when you consider Python int objects made of many "Python digits".)

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Jun 4, 2019

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Modules/mathmodule.c Outdated Show resolved Hide resolved
@niklasf niklasf force-pushed the niklasf:bpo-31031-consolidate-bit-length branch 2 times, most recently from f225c8e to dcaa962 Jun 5, 2019
@niklasf

This comment has been minimized.

Copy link
Contributor Author

niklasf commented Jun 5, 2019

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

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Jun 5, 2019

Thanks for making the requested changes!

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

5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5
};

unsigned int _Py_bit_length(unsigned long d) {

This comment has been minimized.

Copy link
@vstinner

vstinner Jun 5, 2019

Member

Would it make sense to convert this function to an static inline function only exposed in the internal C API?

This comment has been minimized.

Copy link
@mdickinson

mdickinson Jun 5, 2019

Member

For sure, the declaration in pymath.h should be protected by an #ifndef Py_LIMITED_API. @vstinner: I'm not sure I'm understanding you correctly, though. Making this static would make it unavailable to the other files that need it, wouldn't it?

This comment has been minimized.

Copy link
@mdickinson

mdickinson Jun 5, 2019

Member

Ah, I see: you're suggesting moving the entire function definition to pymath.h, and making it static inline. Is that right?

This comment has been minimized.

Copy link
@vstinner

vstinner Jun 5, 2019

Member

Right. Sorry, my comment was unclear. I converted some macros to static inline functions in header files.

I don't know if it would be worth it in term of performance.

This comment has been minimized.

Copy link
@mdickinson

mdickinson Jun 6, 2019

Member

Could be worth a try; I'd be surprised if it makes a measurable difference, but we won't know without testing.

But if not, we do need the Py_LIMITED_API guard: we don't want to expose _Py_bit_length to 3rd party libraries.

@vstinner vstinner added the skip news label Jun 7, 2019
Include/pymath.h Show resolved Hide resolved
@niklasf niklasf force-pushed the niklasf:bpo-31031-consolidate-bit-length branch from dcaa962 to 7b4198c Jun 12, 2019
@csabella csabella requested review from vstinner and mdickinson Jan 16, 2020
Copy link
Member

vstinner left a comment

LGTM.

@vstinner vstinner merged commit c5b7900 into python:master Jan 16, 2020
5 checks passed
5 checks passed
Azure Pipelines PR #20190612.47 succeeded
Details
bedevere/issue-number Issue number 31031 found
Details
bedevere/news "skip news" label found
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Jan 16, 2020

Thanks @niklasf, sorry for slow reviews and for being so pedantic :-) But it was worth it, I like the final change ;-)

I looked again at rejected https://bugs.python.org/issue29782 and #594 I was going to ask further change on this PR to prepare the code in case if someone wants to experiment again more efficient functions... But nah, the code is ok and can be updated later.

petdance added a commit to petdance/cpython that referenced this pull request Jan 17, 2020
Add _Py_bit_length() to unify duplicate bits_in_digit() and bit_length().
petdance added a commit to petdance/cpython that referenced this pull request Jan 17, 2020
Add _Py_bit_length() to unify duplicate bits_in_digit() and bit_length().
petdance added a commit to petdance/cpython that referenced this pull request Jan 17, 2020
Add _Py_bit_length() to unify duplicate bits_in_digit() and bit_length().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.