-
-
Notifications
You must be signed in to change notification settings - Fork 34.5k
bpo-37986: Improve perfomance of PyLong_FromDouble() #15611
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Use strict bound check for safety and symmetry
- Loading branch information
commit 0b90ead7d23be2fced6b547f4d6426a902e04a7f
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
int_maxis an imprecise value on platforms where sizeof(long) >= sizeof(double). Most 64-bit systems have long's larger than a double's 53-bit mantissa (and likely all platforms when considering long long per the above comment).Will it be truncated in the right direction (towards zero) to avoid this triggering on values with undefined conversion behavior?
the previous code used
LONG_MIN < vandv < LONG_MAXdirectly rather than LONG_MAX + 1 stored into a double. (I believe the C promotion will promoted those values to a double before comparison as all floating point types have a higher rank than integer types)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.
The original comment explains why you should use
< LONG_MAX. I would keep the original comment and the code, and just move it intoPyLong_FromDouble().Uh oh!
There was an error while loading. Please reload this page.
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 think I had to add comment about this: I assumed that LONG_MAX == 2 ** (CHAR_BIT * sizeof(long) - 1) - 1 and LONG_MIN == -2 ** (CHAR_BIT * sizeof(long) - 1), i.e.
(unsigned long)LONG_MAX + 1is a power of two and can be exactly represented by double (assuming that FLT_RADIX == 2). Does that make sense?(Originally I wrote it like this:
const double int_max = pow(2, CHAR_BIT * sizeof(long) - 1), see #15611 (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.
Here I'm trying to demonstrate correctness of this approach:
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 think this is fine, under reasonable assumptions on the platform.
LONG_MAX + 1must be a power of 2 (follows from C99 §6.2.6.2p2), and while it's theoretically possible thatdoublewill be unable to representLONG_MAX + 1exactly, that seems highly unlikely in practice. So the conversion todoublemust be exact (C99 §6.3.1.4p2).It's not safe based purely on the C standard to assume that
LONG_MIN = -LONG_MAX - 1: the integer representation could be ones' complement or sign-magnitude, in which caseLONG_MIN = -LONG_MAX. But that assumption is safe in practice for any platform that Python's likely to meet, and we make the assumption of two's complement for signed integers elsewhere in the codebase. If we're worried enough about this, we could change the-int_max <= dvalcomparison to-int_max < dval. On balance, I'd suggest making that change (partly just for the aesthetics of the symmetry).Believe it or not, it's also not safe based purely on the C standard to assume that
(unsigned long)LONG_MAX + 1is representable as anunsigned long: C99 §6.2.5p9 only guarantees that nonnegativelongvalues are representable asunsigned longBut the chance of that not being true in practice is negligible (at least until someone tries to port CPython to the DS9000). And the failure mode is benign: we'd just end up never taking the fast path.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.
Re-reading all this, I had one more worry (which is why I dismissed my own review): what happens if the exact value of
dvallies strictly betweenLONG_MAXandLONG_MAX + 1? In that case we could end up converting adoublethat, strictly speaking, is outside the range oflong. But it turns out that we're safe, because C99 is quite explicit here: §6.3.1.4p1 says (emphasis mine):So any
doublevalue that's strictly smaller thanLONG_MAX + 1should be fine.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.
Then I think we could use
((double)(LONG_MAX / 2 + 1)) * 2, but does it worth it?Shouldn't we formally state that we support only two's complement representation?
BTW it was proposed to abandon other representations and it looks like committee is agree with that.
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.
Definitely not worth it! The C standard permits
LONG_MAX == ULONG_MAX, but I'd be astonished if you ever find a real implementation (now or in the future) that has this property.Yes, we should, though I'm not sure where would be the best place. But I think it's a non-issue in practice.