Skip to content
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.

Sign up for GitHub

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

Jump to bottom

gh-111962: Make dtoa thread-safe in --disable-gil builds. #112049

Merged
merged 5 commits into from
Dec 7, 2023
Merged

gh-111962: Make dtoa thread-safe in --disable-gil builds. #112049

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d53c7f4
gh-111962: Make dtoa thread-safe in `--disable-gil` builds.
colesbury Nov 13, 2023
4de2fb8
Fix size of cached powers of 5 array.
colesbury Nov 13, 2023
893f264
Rename Py_NOGIL to Py_GIL_DISABLED
colesbury Nov 20, 2023
4907b59
Changes from review
colesbury Nov 27, 2023
d9536e5
Fix assertion placement
colesbury Nov 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions Include/internal/pycore_dtoa.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed it to Bigint_Pow5size


#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 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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;
Expand All @@ -57,16 +60,18 @@ struct _dtoa_state {
#endif // !Py_USING_MEMORY_DEBUGGER


/* These functions are used by modules compiled as C extension like math:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted because the comment is out of date, presumably? 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, these comments were about why the below functions used PyAPI_FUNC (were "exported"), but the symbols are no longer exported.

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);
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
Expand Down
71 changes: 46 additions & 25 deletions Python/dtoa.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_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 ==
Expand Down Expand Up @@ -428,7 +428,7 @@ Bfree(Bigint *v)
}
}

#endif /* 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))
Expand Down Expand Up @@ -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 };

Expand All @@ -685,19 +685,12 @@ pow5mult(Bigint *b, int k)

if (!(k >>= 2))
return b;
assert(k < (1 << (Bigint_Pow5max)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 like format(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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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);
Expand All @@ -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;
}
Expand Down Expand Up @@ -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**512
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
}
6 changes: 6 additions & 0 deletions Python/pylifecycle.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down Expand Up @@ -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);
Expand Down
Loading