From d53c7f49c38bddb7bf4205a2f55d945d17154490 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Mon, 13 Nov 2023 14:16:15 -0500 Subject: [PATCH 1/5] gh-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. --- Include/internal/pycore_dtoa.h | 15 ++++--- Python/dtoa.c | 71 ++++++++++++++++++++++------------ Python/pylifecycle.c | 6 +++ 3 files changed, 62 insertions(+), 30 deletions(-) diff --git a/Include/internal/pycore_dtoa.h b/Include/internal/pycore_dtoa.h index ac62a4d300720a..a5f34f7c26bcfd 100644 --- a/Include/internal/pycore_dtoa.h +++ b/Include/internal/pycore_dtoa.h @@ -35,6 +35,9 @@ 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 7 + #ifndef PRIVATE_MEM #define PRIVATE_MEM 2304 #endif @@ -42,9 +45,9 @@ struct _dtoa_state { ((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 */ + struct Bigint *p5s[Bigint_Pow5max]; // XXX This should be freed during runtime fini. - struct Bigint *p5s; struct Bigint *freelist[Bigint_Kmax+1]; double preallocated[Bigint_PREALLOC_SIZE]; double *preallocated_next; @@ -57,9 +60,6 @@ struct _dtoa_state { #endif // !Py_USING_MEMORY_DEBUGGER -/* These functions are used by modules compiled as C extension like math: - they must be exported. */ - extern double _Py_dg_strtod(const char *str, char **ptr); extern char* _Py_dg_dtoa(double d, int mode, int ndigits, int *decpt, int *sign, char **rve); @@ -67,6 +67,11 @@ extern void _Py_dg_freedtoa(char *s); #endif // _PY_SHORT_FLOAT_REPR == 1 + +extern PyStatus _PyDtoa_Init(PyInterpreterState *interp); +extern void _PyDtoa_Fini(PyInterpreterState *interp); + + #ifdef __cplusplus } #endif diff --git a/Python/dtoa.c b/Python/dtoa.c index 5dfc0e179cbc34..9a5a025ee34635 100644 --- a/Python/dtoa.c +++ b/Python/dtoa.c @@ -309,7 +309,7 @@ BCinfo { // struct Bigint is defined in pycore_dtoa.h. typedef struct Bigint Bigint; -#ifndef Py_USING_MEMORY_DEBUGGER +#if !defined(Py_NOGIL) && !defined(Py_USING_MEMORY_DEBUGGER) /* Memory management: memory is allocated from, and returned to, Kmax+1 pools of memory, where pool k (0 <= k <= Kmax) is for Bigints b with b->maxwds == @@ -428,7 +428,7 @@ Bfree(Bigint *v) } } -#endif /* Py_USING_MEMORY_DEBUGGER */ +#endif /* !defined(Py_NOGIL) && !defined(Py_USING_MEMORY_DEBUGGER) */ #define Bcopy(x,y) memcpy((char *)&x->sign, (char *)&y->sign, \ y->wds*sizeof(Long) + 2*sizeof(int)) @@ -673,7 +673,7 @@ mult(Bigint *a, Bigint *b) static Bigint * pow5mult(Bigint *b, int k) { - Bigint *b1, *p5, *p51; + Bigint *b1, *p5, **p5s; int i; static const int p05[3] = { 5, 25, 125 }; @@ -685,19 +685,12 @@ pow5mult(Bigint *b, int k) if (!(k >>= 2)) return b; + assert(k < (1 << (Bigint_Pow5max))); PyInterpreterState *interp = _PyInterpreterState_GET(); - p5 = interp->dtoa.p5s; - if (!p5) { - /* first time */ - p5 = i2b(625); - if (p5 == NULL) { - Bfree(b); - return NULL; - } - interp->dtoa.p5s = p5; - p5->next = 0; - } + p5s = interp->dtoa.p5s; for(;;) { + p5 = *p5s; + p5s++; if (k & 1) { b1 = mult(b, p5); Bfree(b); @@ -707,17 +700,6 @@ pow5mult(Bigint *b, int k) } if (!(k >>= 1)) break; - p51 = p5->next; - if (!p51) { - p51 = mult(p5,p5); - if (p51 == NULL) { - Bfree(b); - return NULL; - } - p51->next = 0; - p5->next = p51; - } - p5 = p51; } return b; } @@ -2811,3 +2793,42 @@ _Py_dg_dtoa(double dd, int mode, int ndigits, } #endif // _PY_SHORT_FLOAT_REPR == 1 + +PyStatus +_PyDtoa_Init(PyInterpreterState *interp) +{ +#if _PY_SHORT_FLOAT_REPR == 1 && !defined(Py_USING_MEMORY_DEBUGGER) + Bigint **p5s = interp->dtoa.p5s; + + // 5**4 = 625 + Bigint *p5 = i2b(625); + if (p5 == NULL) { + return PyStatus_NoMemory(); + } + p5s[0] = p5; + + // compute 5**8, 5**16, 5**32, ..., 5**256 + for (Py_ssize_t i = 1; i < Bigint_Pow5max; i++) { + p5 = mult(p5, p5); + if (p5 == NULL) { + return PyStatus_NoMemory(); + } + p5s[i] = p5; + } + +#endif + return PyStatus_Ok(); +} + +void +_PyDtoa_Fini(PyInterpreterState *interp) +{ +#if _PY_SHORT_FLOAT_REPR == 1 && !defined(Py_USING_MEMORY_DEBUGGER) + Bigint **p5s = interp->dtoa.p5s; + for (Py_ssize_t i = 0; i < Bigint_Pow5max; i++) { + Bigint *p5 = p5s[i]; + p5s[i] = NULL; + Bfree(p5); + } +#endif +} diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index ac8d5208322882..b85e2d7f3cd3ef 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -825,6 +825,11 @@ pycore_interp_init(PyThreadState *tstate) return status; } + status = _PyDtoa_Init(interp); + if (_PyStatus_EXCEPTION(status)) { + return status; + } + // The GC must be initialized before the first GC collection. status = _PyGC_Init(interp); if (_PyStatus_EXCEPTION(status)) { @@ -1781,6 +1786,7 @@ finalize_interp_clear(PyThreadState *tstate) _PyXI_Fini(tstate->interp); _PyExc_ClearExceptionGroupType(tstate->interp); _Py_clear_generic_types(tstate->interp); + _PyDtoa_Fini(tstate->interp); /* Clear interpreter state and all thread states */ _PyInterpreterState_Clear(tstate); From 4de2fb8b3db47d8098ff8f8458fe6d6f8d3c0de0 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Mon, 13 Nov 2023 16:36:52 -0500 Subject: [PATCH 2/5] 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). --- Include/internal/pycore_dtoa.h | 2 +- Python/dtoa.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Include/internal/pycore_dtoa.h b/Include/internal/pycore_dtoa.h index a5f34f7c26bcfd..c2cf71ffd38835 100644 --- a/Include/internal/pycore_dtoa.h +++ b/Include/internal/pycore_dtoa.h @@ -36,7 +36,7 @@ struct _dtoa_state { #define Bigint_Kmax 7 /* The size of the cached powers of 5 array */ -#define Bigint_Pow5max 7 +#define Bigint_Pow5max 8 #ifndef PRIVATE_MEM #define PRIVATE_MEM 2304 diff --git a/Python/dtoa.c b/Python/dtoa.c index 9a5a025ee34635..b29e1aacd96de7 100644 --- a/Python/dtoa.c +++ b/Python/dtoa.c @@ -2807,7 +2807,7 @@ _PyDtoa_Init(PyInterpreterState *interp) } p5s[0] = p5; - // compute 5**8, 5**16, 5**32, ..., 5**256 + // compute 5**8, 5**16, 5**32, ..., 5**512 for (Py_ssize_t i = 1; i < Bigint_Pow5max; i++) { p5 = mult(p5, p5); if (p5 == NULL) { From 893f264d5b0f9264e948df88b2f1c00ccfa2b7b7 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Mon, 20 Nov 2023 10:44:50 -0500 Subject: [PATCH 3/5] Rename Py_NOGIL to Py_GIL_DISABLED --- Python/dtoa.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Python/dtoa.c b/Python/dtoa.c index b29e1aacd96de7..db0b129c1a9e2b 100644 --- a/Python/dtoa.c +++ b/Python/dtoa.c @@ -309,7 +309,7 @@ BCinfo { // struct Bigint is defined in pycore_dtoa.h. typedef struct Bigint Bigint; -#if !defined(Py_NOGIL) && !defined(Py_USING_MEMORY_DEBUGGER) +#if !defined(Py_GIL_DISABLED) && !defined(Py_USING_MEMORY_DEBUGGER) /* Memory management: memory is allocated from, and returned to, Kmax+1 pools of memory, where pool k (0 <= k <= Kmax) is for Bigints b with b->maxwds == @@ -428,7 +428,7 @@ Bfree(Bigint *v) } } -#endif /* !defined(Py_NOGIL) && !defined(Py_USING_MEMORY_DEBUGGER) */ +#endif /* !defined(Py_GIL_DISABLED) && !defined(Py_USING_MEMORY_DEBUGGER) */ #define Bcopy(x,y) memcpy((char *)&x->sign, (char *)&y->sign, \ y->wds*sizeof(Long) + 2*sizeof(int)) From 4907b5988cedf125dd875685e47490c8047428a6 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Mon, 27 Nov 2023 10:49:14 -0500 Subject: [PATCH 4/5] Changes from review --- Include/internal/pycore_dtoa.h | 7 ++++--- Python/dtoa.c | 13 ++++++++++--- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/Include/internal/pycore_dtoa.h b/Include/internal/pycore_dtoa.h index c2cf71ffd38835..c5cfdf4ce8f823 100644 --- a/Include/internal/pycore_dtoa.h +++ b/Include/internal/pycore_dtoa.h @@ -36,7 +36,7 @@ struct _dtoa_state { #define Bigint_Kmax 7 /* The size of the cached powers of 5 array */ -#define Bigint_Pow5max 8 +#define Bigint_Pow5size 8 #ifndef PRIVATE_MEM #define PRIVATE_MEM 2304 @@ -45,8 +45,9 @@ struct _dtoa_state { ((PRIVATE_MEM+sizeof(double)-1)/sizeof(double)) struct _dtoa_state { - /* p5s is an array of powers of 5 of the form 5**(2**i), i >= 2 */ - struct Bigint *p5s[Bigint_Pow5max]; + // p5s is an array of powers of 5 of the form: + // 5**(2**(i+2)) for 0 <= i < Bigint_Pow5size + struct Bigint *p5s[Bigint_Pow5size]; // XXX This should be freed during runtime fini. struct Bigint *freelist[Bigint_Kmax+1]; double preallocated[Bigint_PREALLOC_SIZE]; diff --git a/Python/dtoa.c b/Python/dtoa.c index db0b129c1a9e2b..f917aa37f34ce5 100644 --- a/Python/dtoa.c +++ b/Python/dtoa.c @@ -677,6 +677,13 @@ pow5mult(Bigint *b, int k) int i; static const int p05[3] = { 5, 25, 125 }; + // For double-to-string conversion, the maximum value of k is limited by + // DBL_MAX_10_EXP (308), the maximum decimal base-10 exponent for binary64. + // For string-to-double conversion, the extreme case is constrained by our + // hardcoded exponent limit before we underflow of -512, adjusted by + // STRTOD_DIGLIM-DBL_DIG-1, giving a maximum of k=535. + assert(0 <= k && k < 1024); + if ((i = k & 3)) { b = multadd(b, p05[i-1], 0); if (b == NULL) @@ -685,12 +692,12 @@ pow5mult(Bigint *b, int k) if (!(k >>= 2)) return b; - assert(k < (1 << (Bigint_Pow5max))); PyInterpreterState *interp = _PyInterpreterState_GET(); p5s = interp->dtoa.p5s; for(;;) { p5 = *p5s; p5s++; + assert(p5s != interp->dtoa.p5s + Bigint_Pow5size); if (k & 1) { b1 = mult(b, p5); Bfree(b); @@ -2808,7 +2815,7 @@ _PyDtoa_Init(PyInterpreterState *interp) p5s[0] = p5; // compute 5**8, 5**16, 5**32, ..., 5**512 - for (Py_ssize_t i = 1; i < Bigint_Pow5max; i++) { + for (Py_ssize_t i = 1; i < Bigint_Pow5size; i++) { p5 = mult(p5, p5); if (p5 == NULL) { return PyStatus_NoMemory(); @@ -2825,7 +2832,7 @@ _PyDtoa_Fini(PyInterpreterState *interp) { #if _PY_SHORT_FLOAT_REPR == 1 && !defined(Py_USING_MEMORY_DEBUGGER) Bigint **p5s = interp->dtoa.p5s; - for (Py_ssize_t i = 0; i < Bigint_Pow5max; i++) { + for (Py_ssize_t i = 0; i < Bigint_Pow5size; i++) { Bigint *p5 = p5s[i]; p5s[i] = NULL; Bfree(p5); From d9536e5db1d918925b44641b7576461ad8271611 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Mon, 27 Nov 2023 11:06:46 -0500 Subject: [PATCH 5/5] Fix assertion placement --- Python/dtoa.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Python/dtoa.c b/Python/dtoa.c index f917aa37f34ce5..6e3162f80bdae1 100644 --- a/Python/dtoa.c +++ b/Python/dtoa.c @@ -695,9 +695,9 @@ pow5mult(Bigint *b, int k) PyInterpreterState *interp = _PyInterpreterState_GET(); p5s = interp->dtoa.p5s; for(;;) { + assert(p5s != interp->dtoa.p5s + Bigint_Pow5size); p5 = *p5s; p5s++; - assert(p5s != interp->dtoa.p5s + Bigint_Pow5size); if (k & 1) { b1 = mult(b, p5); Bfree(b);