gh-111962: Make dtoa thread-safe in --disable-gil builds.
#112049
Conversation
This avoids using the Bigint free-list in `--disable-gil` builds and pre-computes the needed powers of 5 during interpreter initialization.
We need the powers of 5 up to 5**512 because we only jump straight to underflow when the exponent is less than -512 (or larger than 308).
colesbury
force-pushed
the
dtoa-thread-safety
branch
from
4fd7747
to
4de2fb8
Compare
Include/internal/pycore_dtoa.h
Outdated
| @@ -35,16 +35,19 @@ struct _dtoa_state { | |||
| /* The size of the Bigint freelist */ | |||
| #define Bigint_Kmax 7 | |||
|
|
|||
| /* The size of the cached powers of 5 array */ | |||
| #define Bigint_Pow5max 8 | |||
There was a problem hiding this comment.
Nit: I'm wondering whether there's a better name here; the "max" part seems potentially confusing (the max value stored here would be 5**2**9, right?). Bigint_Pow5count? Bigint_Pow5size?
There was a problem hiding this comment.
I renamed it to Bigint_Pow5size
Include/internal/pycore_dtoa.h
Outdated
| #ifndef PRIVATE_MEM | ||
| #define PRIVATE_MEM 2304 | ||
| #endif | ||
| #define Bigint_PREALLOC_SIZE \ | ||
| ((PRIVATE_MEM+sizeof(double)-1)/sizeof(double)) | ||
|
|
||
| struct _dtoa_state { | ||
| /* p5s is a linked list of powers of 5 of the form 5**(2**i), i >= 2 */ | ||
| /* p5s is an array of powers of 5 of the form 5**(2**i), i >= 2 */ |
There was a problem hiding this comment.
Nit: Could we adjust this comment to be more explicit about the indexing - i.e., make it clear that p5s[i] is 5**(2**(i+2)) for 0 <= i < Bigint_Pow5max?
| @@ -57,16 +60,18 @@ struct _dtoa_state { | |||
| #endif // !Py_USING_MEMORY_DEBUGGER | |||
|
|
|||
|
|
|||
| /* These functions are used by modules compiled as C extension like math: | |||
There was a problem hiding this comment.
Deleted because the comment is out of date, presumably? 👍
There was a problem hiding this comment.
Yeah, these comments were about why the below functions used PyAPI_FUNC (were "exported"), but the symbols are no longer exported.
Python/dtoa.c
Outdated
| @@ -685,19 +685,12 @@ pow5mult(Bigint *b, int k) | |||
|
|
|||
| if (!(k >>= 2)) | |||
| return b; | |||
| assert(k < (1 << (Bigint_Pow5max))); | |||
There was a problem hiding this comment.
This single line is by far the hardest to review, and reminds me yet again why I'd be delighted to see dtoa.c go away entirely. :-)
This says that pow5mult will never be called with k >= 1024, right?
I do believe that there's an upper limit on possible k values here, based on:
- For double-to-string conversion, our input space is naturally bounded, and since we're assuming IEEE 754 binary64 format, we have at most 767 significant decimal digits in any output (an example worst case is something like
(2**53 - 1) * 2**-1074), and dtoa.c is clever enough to pad with zeros when requested (e.g., with something crazy likeformat(math.pi, '.2000f')) rather than try to compute those zeros. - For string-to-double conversion, the input string has a potentially unbounded number of decimal digits, all of which may need to be taken into account for correct-rounding corner cases, but in those corner cases dtoa.c (in bigcomp) takes the approach of computing decimal digits of binary64 tie values and comparing with the decimal input, rather than trying to convert the decimal input to bigint-land (which would potentially require huge powers of 5).
And based on the above, 1023 certainly seems plausible as an upper bound for possible k values. I'd be hard pushed to give a proof that k can never exceed 1023, though. (At least, not without spending a lot more time than I currently have available.)
@colesbury Does the above roughly match your reasoning? Would it be possible to add a comment to the code justifying the assertion?
There was a problem hiding this comment.
I've added a comment and moved the assertion up. The limits are related to the maximum base-10 exponent. For double-to-string, that's DBL_MAX_10_EXP (308). As you say, we set the limits ourselves for string-to-double, which are e=308 for overflow and e=-512 for underflow. But our exponent can be adjusted based on the number of digits, so we can see values as larges as k=535 float('1' +'0'*38 + '1E-535'), where the limits is from the combination of 512+STRTOD_DIGLIM-DBL_DIG-1.
|
@mdickinson - would you please merge this? |
|
@colesbury Apologies; I'd been assuming you'd merge it (and was beginning to wonder why you hadn't). :-) |
Probably Sam doesn't have rights to merge |
…thon#112049) This updates `dtoa.c` to avoid using the Bigint free-list in --disable-gil builds and to pre-computes the needed powers of 5 during interpreter initialization. * pythongh-111962: Make dtoa thread-safe in `--disable-gil` builds. This avoids using the Bigint free-list in `--disable-gil` builds and pre-computes the needed powers of 5 during interpreter initialization. * Fix size of cached powers of 5 array. We need the powers of 5 up to 5**512 because we only jump straight to underflow when the exponent is less than -512 (or larger than 308). * Rename Py_NOGIL to Py_GIL_DISABLED * Changes from review * Fix assertion placement
…thon#112049) This updates `dtoa.c` to avoid using the Bigint free-list in --disable-gil builds and to pre-computes the needed powers of 5 during interpreter initialization. * pythongh-111962: Make dtoa thread-safe in `--disable-gil` builds. This avoids using the Bigint free-list in `--disable-gil` builds and pre-computes the needed powers of 5 during interpreter initialization. * Fix size of cached powers of 5 array. We need the powers of 5 up to 5**512 because we only jump straight to underflow when the exponent is less than -512 (or larger than 308). * Rename Py_NOGIL to Py_GIL_DISABLED * Changes from review * Fix assertion placement
This avoids using the Bigint free-list in
--disable-gilbuilds and pre-computes the needed powers of 5 during interpreter initialization.--disable-gilbuilds #111962