-
-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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-37907: Slightly improve performance of PyLong_AsSsize_t() with large longs #15363
base: main
Are you sure you want to change the base?
Conversation
ec8c2c0
to
7147451
Compare
@@ -0,0 +1,3 @@ | |||
Slightly improve performance of :c:func:`PyLong_AsSsize_t`, |
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.
This is a completely trivial internal change. I don't think it needs a NEWS
entry.
Objects/longobject.c
Outdated
prev = x; | ||
x = (x << PyLong_SHIFT) | v->ob_digit[i]; | ||
if ((x >> PyLong_SHIFT) != prev) { | ||
if (x > (unsigned long)-1 >> PyLong_SHIFT) { |
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.
It would be good to add a comment here (and other similar lines below). Something along the lines of
/* The right hand of this comparison if the largest unsigned long value
* that can be shifted left by PyLong_SHIFT bits without overflow. */
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.
IMO it's quite obvious from the code.
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 would prefer ULONG_MAX rather than (unsigned long)-1. Same comment for (size_t)-1 below.
7147451
to
6d0996d
Compare
Objects/longobject.c
Outdated
prev = x; | ||
x = (x << PyLong_SHIFT) | v->ob_digit[i]; | ||
if ((x >> PyLong_SHIFT) != prev) { | ||
if (x > (unsigned long)-1 >> PyLong_SHIFT) { |
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 would prefer ULONG_MAX rather than (unsigned long)-1. Same comment for (size_t)-1 below.
@@ -514,12 +514,11 @@ PyLong_AsLongAndOverflow(PyObject *vv, int *overflow) | |||
i = -(i); | |||
} |
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.
Would it be possible to start at "x = v->ob_digit[0];" and "i=1;" to avoid starting the loop with x=0 which means one useless if at the first iteration.
With 64-bit unsigned long and PyLong_SHIFT, we can even combine two digits without having to check for overflow, no?
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.
we can even combine two digits without having to check for overflow, no?
Something like:
/* in the default: case, i >= 2 */
assert(i >= 2);
#if ((ULONG_MAX >> PyLong_SHIFT)) >= ((1UL << PyLong_SHIFT) - 1)
/* use 2 digits */
--i;
x= digit[i];
x <<=PyLong_SHIFT;
--i;
x |= digit[i];
#else
/* use 1 digit */
--i
assert(ULONG_MAX >= ((1UL << PyLong_SHIFT) - 1);
x= digit[i];
#endif
while (--i >= 0) { ... }
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 |
There is a similar overflow check in |
Is this pull request supposed to be closed? I'm still getting speed-ups when implemented in the current version of Python. |
It's awaiting changes from @sir-sigurd. |
d7a3286
to
97a1edd
Compare
@sir-sigurd Changes look good to me, but the PR needs a rebase. |
https://bugs.python.org/issue37907