bpo-40521: [subinterpreters] Make dtoa bigint free list per-interpreter #24821

JunyiXie · 2021-03-11T09:43:25Z

https://bugs.python.org/issue40521
Make dtoa bigint free list per-interpreter

https://bugs.python.org/issue40521

vstinner · 2021-03-11T14:46:57Z

Include/internal/pycore_dtoa.h

+
+typedef uint32_t ULong;
+typedef int32_t Long;
+typedef uint64_t ULLong;


Many C files include directly or indirectly pycore_interp.h which includes pycore_dtoa.h. I would prefer to not pollule their namespaces with names like Kmax, ULong, Long, UULong which can clash with other names.

I see different options:

Don't fix dtoa.c for now

Add a prefix to all these names in the header file (ex: "PyDtoa") and create aliases in dtoa.c (ex: #define Kmax _PyDtoa_Kmax).

Add an API like PyInterpreterState.dict to have per-interpreter variables: the freelist would be dynamically allocated on the heap memory, and stored as a raw pointer in PyInterpreterState. Or if you don't want to add a whole API, just expose the cache as a "void *" in the structure, and add init/fini functions to allocate/free this structure (free list).

thanks for your review. I chose the second option to fix this problem.

bedevere-bot · 2021-03-11T14:47:04Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

JunyiXie · 2021-03-12T03:59:12Z

I have made the requested changes; please review again

bedevere-bot · 2021-03-12T03:59:14Z

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

JunyiXie · 2021-03-12T04:00:10Z

I have removed the news. But I don't know how to make bedevere/news — No news entry in Misc/NEWS.d/next/ or "skip news" label found check success.

vstinner · 2021-03-12T11:05:23Z

Include/internal/pycore_dtoa.h

+    _PyDtoa_ULong x[1];
+};
+
+typedef struct Bigint Bigint;


This typedef is not needed in pycore_interp.h, please move it into dtoa.c.

vstinner · 2021-03-12T11:06:14Z

Include/internal/pycore_dtoa.h

+struct
+Bigint {
+    struct Bigint *next;


Suggested change

struct

Bigint {

struct Bigint *next;

struct _PyDtoa_Bigint {

struct _PyDtoa_Bigint *next;

vstinner · 2021-03-12T11:06:38Z

Include/internal/pycore_interp.h

@@ -321,6 +322,7 @@ struct _is {

    struct ast_state ast;
    struct type_cache type_cache;
+    Bigint *bigint_freelist[_PyDtoa_Kmax+1];


Suggested change

Bigint *bigint_freelist[_PyDtoa_Kmax+1];

struct _PyDtoa_Bigint *dtoa_freelist[_PyDtoa_Kmax + 1];

vstinner · 2021-03-12T11:07:38Z

Python/dtoa.c

-
-    if (k <= Kmax && (rv = freelist[k]))
-        freelist[k] = rv->next;
+    PyInterpreterState *is = PyInterpreterState_Get();


I would prefer to add a helper function get_freelist().

I suggest to use _PyInterpreterState_GET() which is faster than PyInterpreterState_Get().

vstinner · 2021-03-12T11:08:02Z

Python/dtoa.c

+#define Kmax _PyDtoa_Kmax
+#define ULong _PyDtoa_ULong
+#define Long _PyDtoa_Long
+#define ULLong _PyDtoa_ULLong


Suggested change

#define Kmax _PyDtoa_Kmax

#define ULong _PyDtoa_ULong

#define Long _PyDtoa_Long

#define ULLong _PyDtoa_ULLong

#define ULong _PyDtoa_ULong

#define Long _PyDtoa_Long

#define ULLong _PyDtoa_ULLong

#define Kmax _PyDtoa_Kmax

typedef struct _PyDtoa_Bigint Bigint;

vstinner · 2021-03-12T11:10:00Z

Include/internal/pycore_dtoa.h

@@ -1,4 +1,6 @@
 #ifndef PY_NO_SHORT_FLOAT_REPR
+#ifndef Py_CORE_DTOA_H


To follow other files, you should use "INTERNAL" instead: #ifndef Py_INTERNAL_DTOA_H.

vstinner · 2021-03-12T11:12:01Z

Include/internal/pycore_interp.h

@@ -321,6 +322,7 @@ struct _is {

    struct ast_state ast;
    struct type_cache type_cache;
+    Bigint *bigint_freelist[_PyDtoa_Kmax+1];


You need to add:

#ifndef PY_NO_SHORT_FLOAT_REPR ... #endif

If the PY_NO_SHORT_FLOAT_REPR macro is defined, dtoa.c is not used.

JunyiXie · 2021-03-12T13:09:47Z

Thank vstinner, for your very detailed review. I did not consider these details before.

JunyiXie · 2021-03-12T13:15:08Z

I have made the requested changes; please review again

bedevere-bot · 2021-03-12T13:15:10Z

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

vstinner · 2021-03-12T16:28:23Z

Python/dtoa.c

+
+/* Get Bigint freelist from interpreter  */
+static Bigint **
+get_freelist() {


See "function declaration isn’t a prototype [-Wstrict-prototypes]" compiler warning:

Suggested change

get_freelist() {

get_freelist(void) {

Thank you for your review, sorry I didn't pay attention to the warning.

JunyiXie · 2021-03-12T16:47:08Z

I have made the requested changes; please review again

bedevere-bot · 2021-03-12T16:47:10Z

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

vstinner · 2021-03-12T21:30:53Z

Can you please build Python with PY_NO_SHORT_FLOAT_REPR macro defined? For example, add -DPY_NO_SHORT_FLOAT_REPR=1 flag in CFLAGS.

JunyiXie · 2021-03-13T04:47:58Z

@vstinner Thank you for your reminder, I added -DPY_NO_SHORT_FLOAT_REPR=1 to run the unit test.

11 tests failed again:
     test_builtin test_configparser test_ctypes test_difflib test_float
     test_format test_fstring test_optparse test_statistics test_types
     test_unicode

======================================================================
FAIL: test_defaults_keyword (test.test_configparser.ConfigParserTestCase)
[bpo-23835](https://bugs.python.org/issue23835) fix for ConfigParser
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/xiejunyi/bdcpython/Lib/test/test_configparser.py", line 979, in test_defaults_keyword
    self.assertEqual(cf[self.default_section]['1'], '2.4')
AssertionError: '2.3999999999999999' != '2.4'
- 2.3999999999999999
+ 2.4
======================================================================
FAIL: test_defaults_keyword (test.test_configparser.ConfigParserTestCaseLegacyInterpolation)
[bpo-23835](https://bugs.python.org/issue23835) fix for ConfigParser
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/xiejunyi/bdcpython/Lib/test/test_configparser.py", line 979, in test_defaults_keyword
    self.assertEqual(cf[self.default_section]['1'], '2.4')
AssertionError: '2.3999999999999999' != '2.4'
- 2.3999999999999999
+ 2.4

After I revert my commit and run the unit test as well, there will still be these errors.
I guess defining PY_NO_SHORT_FLOAT_REPR=1 will cause these errors. not my commit cause those test faileds.

vstinner · 2021-03-13T13:26:31Z

I merged your PR thanks.

@vstinner Thank you for your reminder, I added -DPY_NO_SHORT_FLOAT_REPR=1 to run the unit test.

I just wanted to check if it's possible to build Python in the PY_NO_SHORT_FLOAT_REPR macro defined: it's the case, thanks. I'm not interetesed by test_configparser failures. Platforms where PY_NO_SHORT_FLOAT_REPR macro is defined are rare. If an user of one of such platform is impacted, they can report an issue.

…)" This reverts commit 5bd1059.

…)" (GH-24964) This reverts commit 5bd1059.

the-knights-who-say-ni added the CLA signed label Mar 11, 2021

bedevere-bot added the awaiting review label Mar 11, 2021

JunyiXie changed the title ~~issue40521: [subinterpreters] Make dtoa bigint free list per-interpreter~~ bpo-40521: [subinterpreters] Make dtoa bigint free list per-interpreter Mar 11, 2021

vstinner requested changes Mar 11, 2021

View reviewed changes

bedevere-bot removed the awaiting review label Mar 11, 2021

bedevere-bot added the awaiting changes label Mar 11, 2021

bedevere-bot added awaiting change review and removed awaiting changes labels Mar 12, 2021

bedevere-bot requested a review from vstinner March 12, 2021 03:59

vstinner reviewed Mar 12, 2021

View reviewed changes

bedevere-bot requested a review from vstinner March 12, 2021 13:15

vstinner reviewed Mar 12, 2021

View reviewed changes

issue40521: [subinterpreters] Make dtoa bigint free list per-interpreter

90b95e8

bedevere-bot requested a review from vstinner March 12, 2021 16:47

vstinner added the skip news label Mar 13, 2021

vstinner merged commit 5bd1059 into python:master Mar 13, 2021

bedevere-bot removed the awaiting change review label Mar 13, 2021

vstinner added a commit that referenced this pull request Mar 22, 2021

Revert "bpo-40521: Make dtoa bigint free list per-interpreter (GH-24821…

e5dde2c

…)" This reverts commit 5bd1059.

vstinner mentioned this pull request Mar 22, 2021

Revert "bpo-40521: [subinterpreters] Make dtoa bigint free list per-interpreter" #24964

Merged

vstinner added a commit that referenced this pull request Mar 22, 2021

Revert "bpo-40521: Make dtoa bigint free list per-interpreter (GH-24821…

39f6436

…)" (GH-24964) This reverts commit 5bd1059.

vstinner mentioned this pull request Apr 10, 2022

[subinterpreters] Make free lists and unicode caches per-interpreter #84701

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-40521: [subinterpreters] Make dtoa bigint free list per-interpreter #24821

bpo-40521: [subinterpreters] Make dtoa bigint free list per-interpreter #24821

JunyiXie commented Mar 11, 2021 •

edited by bedevere-bot

Loading

vstinner Mar 11, 2021

JunyiXie Mar 12, 2021

bedevere-bot commented Mar 11, 2021

JunyiXie commented Mar 12, 2021

bedevere-bot commented Mar 12, 2021

JunyiXie commented Mar 12, 2021

vstinner Mar 12, 2021

vstinner Mar 12, 2021

vstinner Mar 12, 2021

vstinner Mar 12, 2021

vstinner Mar 12, 2021

vstinner Mar 12, 2021

vstinner Mar 12, 2021

JunyiXie commented Mar 12, 2021

JunyiXie commented Mar 12, 2021

bedevere-bot commented Mar 12, 2021

vstinner Mar 12, 2021

JunyiXie Mar 12, 2021

JunyiXie commented Mar 12, 2021

bedevere-bot commented Mar 12, 2021

vstinner commented Mar 12, 2021

JunyiXie commented Mar 13, 2021 •

edited by bedevere-bot

Loading

vstinner commented Mar 13, 2021

	Bigint *bigint_freelist[_PyDtoa_Kmax+1];
	struct _PyDtoa_Bigint *dtoa_freelist[_PyDtoa_Kmax + 1];

		@@ -1,4 +1,6 @@
		#ifndef PY_NO_SHORT_FLOAT_REPR
		#ifndef Py_CORE_DTOA_H

bpo-40521: [subinterpreters] Make dtoa bigint free list per-interpreter #24821

bpo-40521: [subinterpreters] Make dtoa bigint free list per-interpreter #24821

Conversation

JunyiXie commented Mar 11, 2021 • edited by bedevere-bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Mar 11, 2021

JunyiXie commented Mar 12, 2021

bedevere-bot commented Mar 12, 2021

JunyiXie commented Mar 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunyiXie commented Mar 12, 2021

JunyiXie commented Mar 12, 2021

bedevere-bot commented Mar 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunyiXie commented Mar 12, 2021

bedevere-bot commented Mar 12, 2021

vstinner commented Mar 12, 2021

JunyiXie commented Mar 13, 2021 • edited by bedevere-bot Loading

vstinner commented Mar 13, 2021

JunyiXie commented Mar 11, 2021 •

edited by bedevere-bot

Loading

JunyiXie commented Mar 13, 2021 •

edited by bedevere-bot

Loading