bpo-31031: Unify duplicate bits_in_digit and bit_length #2866

niklasf · 2017-07-25T12:50:41Z

https://bugs.python.org/issue31031

auvipy

check the build errors plz

Include/pymath.h

vstinner · 2019-06-04T14:08:17Z

Include/pymath.h

+ * 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);


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.

(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?

vstinner · 2019-06-04T14:11:22Z

Python/pymath.c

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


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).

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

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.

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 · 2019-06-04T14:11:45Z

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

niklasf · 2019-06-05T10:11:30Z

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

bedevere-bot · 2019-06-05T10:11:33Z

Thanks for making the requested changes!

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

vstinner · 2019-06-05T11:23:15Z

Python/pymath.c

+    5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5
+};
+
+unsigned int _Py_bit_length(unsigned long d) {


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

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?

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

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.

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.

Include/pymath.h

vstinner

LGTM.

vstinner · 2020-01-16T14:11:03Z

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.

Add _Py_bit_length() to unify duplicate bits_in_digit() and bit_length().

In pythonGH-2866, _Py_Bit_Length() was added to pymath.h for lack of a better location. pythonGH-20518 added a more appropriate header file for bit utilities. It also shows how to properly use intrinsics. This allows reconsidering bpo-29782. * Move the function to the new header. * Changed return type to match __builtin_clzl and reviewed usage. * Use intrinsics where available. * Pick a (mostly theoretical) fallback implementation suitable for inlining.

In GH-2866, _Py_Bit_Length() was added to pymath.h for lack of a better location. GH-20518 added a more appropriate header file for bit utilities. It also shows how to properly use intrinsics. This allows reconsidering bpo-29782. * Move the function to the new header. * Changed return type to match __builtin_clzl() and reviewed usage. * Use intrinsics where available. * Pick a fallback implementation suitable for inlining.

In pythonGH-2866, _Py_Bit_Length() was added to pymath.h for lack of a better location. pythonGH-20518 added a more appropriate header file for bit utilities. It also shows how to properly use intrinsics. This allows reconsidering bpo-29782. * Move the function to the new header. * Changed return type to match __builtin_clzl() and reviewed usage. * Use intrinsics where available. * Pick a fallback implementation suitable for inlining.

the-knights-who-say-ni added the CLA signed label Jul 25, 2017

brettcannon added the awaiting review label Feb 2, 2018

auvipy suggested changes Jun 2, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Jun 2, 2019

niklasf force-pushed the bpo-31031-consolidate-bit-length branch from ccdbe59 to 900f7a8 Compare June 2, 2019 14:15

vstinner requested changes Jun 4, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Jun 4, 2019

mdickinson reviewed Jun 4, 2019

View reviewed changes

Modules/mathmodule.c Outdated Show resolved Hide resolved

niklasf force-pushed the bpo-31031-consolidate-bit-length branch 2 times, most recently from f225c8e to dcaa962 Compare June 5, 2019 10:07

bedevere-bot added awaiting change review and removed awaiting changes labels Jun 5, 2019

vstinner reviewed Jun 5, 2019

View reviewed changes

vstinner added the skip news label Jun 7, 2019

vstinner reviewed Jun 7, 2019

View reviewed changes

Include/pymath.h Show resolved Hide resolved

bpo-31031: Unify duplicate bits_in_digit and bit_length

7b4198c

niklasf force-pushed the bpo-31031-consolidate-bit-length branch from dcaa962 to 7b4198c Compare June 12, 2019 14:42

csabella requested review from vstinner and mdickinson January 16, 2020 11:27

vstinner approved these changes Jan 16, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jan 16, 2020

vstinner merged commit c5b7900 into python:master Jan 16, 2020

bedevere-bot removed the awaiting merge label Jan 16, 2020

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020

bpo-31031: Unify duplicate bits_in_digit and bit_length (pythonGH-2866)

ae7bf81

Add _Py_bit_length() to unify duplicate bits_in_digit() and bit_length().

niklasf mentioned this pull request Jun 8, 2020

bpo-29782: Consolidate _Py_Bit_Length() #20739

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-31031: Unify duplicate bits_in_digit and bit_length #2866

bpo-31031: Unify duplicate bits_in_digit and bit_length #2866

niklasf commented Jul 25, 2017 •

edited by bedevere-bot

Loading

auvipy left a comment

vstinner Jun 4, 2019

niklasf Jun 5, 2019

vstinner Jun 4, 2019

mdickinson Jun 4, 2019

niklasf Jun 5, 2019

vstinner Jun 5, 2019

vstinner Jun 5, 2019

bedevere-bot commented Jun 4, 2019

niklasf commented Jun 5, 2019

bedevere-bot commented Jun 5, 2019

vstinner Jun 5, 2019

mdickinson Jun 5, 2019

mdickinson Jun 5, 2019

vstinner Jun 5, 2019

mdickinson Jun 6, 2019

vstinner left a comment

vstinner commented Jan 16, 2020

bpo-31031: Unify duplicate bits_in_digit and bit_length #2866

bpo-31031: Unify duplicate bits_in_digit and bit_length #2866

Conversation

niklasf commented Jul 25, 2017 • edited by bedevere-bot Loading

auvipy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Jun 4, 2019

niklasf commented Jun 5, 2019

bedevere-bot commented Jun 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

vstinner commented Jan 16, 2020

niklasf commented Jul 25, 2017 •

edited by bedevere-bot

Loading