-
-
Notifications
You must be signed in to change notification settings - Fork 33.4k
gh-140036: _pydecimal: avoid slow exponentiation in floor division #140044
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
Open
picnixz
wants to merge
10
commits into
python:main
Choose a base branch
from
picnixz:fix/decimal/infinite-prec-140036
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
ee617bd
_pydecimal: avoid slow exponentiation in floor division
picnixz 329c31a
refine `q < 10**context.prec` checks
picnixz 103fe1e
_pydecimal: add helpers for computing len(str(q)) < a
picnixz efbdc0a
_pydecimal: use helpers for computing len(str(q)) < a
picnixz 9377ab6
_pydecimal: add tests for unbounded contexts
picnixz 23494fb
Apply suggestions from code review
picnixz 6b55711
address review
picnixz 49d5b0d
reformulate comment
picnixz ea3e499
Update Lib/_pydecimal.py
picnixz b1c24b7
add test for _is_less_than_pow10a_use_str() slow path
picnixz 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
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
2 changes: 2 additions & 0 deletions
2
Misc/NEWS.d/next/Library/2025-10-13-15-43-09.gh-issue-140036.b_59uN.rst
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| Avoid hanging in floor division of pure Python :class:`decimal.Decimal` | ||
| instances when the context precision is very large. Patch by Bénédikt Tran. |
Oops, something went wrong.
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.
I think both variants are fine, but you should choose one. You shouldn't worry about string conversion limit.
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.
It was to prevent corner cases. I don't have the time to check, but I feel that it's possible to have some
qsuch thatq.bit_length() < 1 + context.prec * _LOG_10_BASE_2is true butlen(str(q)) <= context.precis false. But maybe this is impossible.I also didn't want to compute
str(q)before as it could be an expensive check. Ifif q.bit_length() < 1 + context.prec * _LOG_10_BASE_2already fails, there is no need to computestr(q)which could also raise an exception.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.
It's impossible if
_LOG_10_BASE_2is less or equal than real mathematical value oflog_2(10)and we change condition toq.bit_length() < context.prec * _LOG_10_BASE_2.Let's prove it:
a)
q.bit_length() < context.prec * _LOG_10_BASE_2is trueBUT
b)
len(str(q)) <= context.precis false.We have
a)
q.bit_length() < context.prec * _LOG_10_BASE_2b)
len(str(q)) > context.prec.Since
len(str(q))andcontext.precare integers, we havelen(str(q)) >= context.prec + 1,and
q >= 10 ** context.prec.Then we apply mathematical
log_2:log_2 (q) >= context.prec * log_2(10), whenlog_2(10)is exact value.Since
log_2(10) >= _LOG_10_BASE_2, we havelog_2 (q) >= context.prec * log_2(10) >= context.prec * _LOG_10_BASE_2 > q.bit_length().As a result,
q > 2 ** q.bit_length()And this is impossible inequation.
It seems that checking of
q.bit_length() < context.prec * _LOG_10_BASE_2would be good enough.If it doesn't, construction of
strlooks like best solution.And some small example:
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.
Thanks for the analysis.
This appears to be the case as
pow(2, float.fromhex('0x1.a934f0979a371p+1'))is something like 9.99...98.Ok, so the
1+was indeed too much (that's what I feared). This will also help in changing theq > 10 ** preccase as well.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.
Michail, thanks for a correction of the first inequality
q.bit_length() < context.prec * _LOG_10_BASE_2(1).Unfortunately, I don't think that this inequality could be used as an "optimization". Remember, it should be an equivalent of
q < 10**context.prec(2). I.e. both inequalities should have same boolean values. In particular, if (1) is false - (2) should be false, as in this case we miss verification by the second check.This equivalency is trivial for
len(str(q)) <= context.prec(3) and (2), whereq >= 0andcontext.prec > 0are integers. I think we should use this, there is no possible short-cut.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.
It's impossible to make equivalent inequality only with
q.bit_length. But we can significantly reduce the needs of calculation oflen(str(q)). We already prove that ifq.bit_length() < context.prec * _LOG_10_BASE_2thenlen(str(q)) <= context.precand there's no need to check. On the other hand, ifq.bit_length() >= 1 + context.prec * _LOG_10_BASE_2_G, thenq >= 2**(q.bit_length()-1) >= 2**(context.prec * _LOG_10_BASE_2_G) >= 2**(context.prec * log_2(10)) = 10**context.prec, where_LOG_10_BASE_2_G = float.fromhex('0x1.a934f0979a372p+1')which is slightly greater then exact value oflog_2(10).It means we could implement checking this way:
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 has more sense for me. Though, IMO such helper not worth if it's required in one or two places in 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.
Yes, I forgot to ask yesterday to check for the reverse condition (I started taking my pen & paper but then had to leave). I'll go for a helper as it's used more than once just for clarity purposes.
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 guess, you are planning yet for another instance in #140036 (comment). Two cases - not too much...
On another hand, we have a lot of len(str) computations in the module:
IMO, keeping code simple is better. Maybe we should apply instead wishful thinking that eventually int->str conversion will be fast.