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

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

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

Suggested change
struct
Bigint{
structBigint*next;
struct_PyDtoa_Bigint{
struct_PyDtoa_Bigint*next;

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

Include/internal/pycore_interp.h

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

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

Suggested change
#defineKmax _PyDtoa_Kmax
#defineULong _PyDtoa_ULong
#defineLong _PyDtoa_Long
#defineULLong _PyDtoa_ULLong
#defineULong _PyDtoa_ULong
#defineLong _PyDtoa_Long
#defineULLong _PyDtoa_ULLong
#defineKmax _PyDtoa_Kmax
typedefstruct_PyDtoa_BigintBigint;

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

Include/internal/pycore_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

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

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-interpreterMar 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:masterMar 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

-struct
-Bigint{
-structBigint*next;
+struct_PyDtoa_Bigint{
+struct_PyDtoa_Bigint*next;

	Bigint*bigint_freelist[_PyDtoa_Kmax+1];
	struct_PyDtoa_Bigint*dtoa_freelist[_PyDtoa_Kmax+1];

-#defineKmax _PyDtoa_Kmax
-#defineULong _PyDtoa_ULong
-#defineLong _PyDtoa_Long
-#defineULLong _PyDtoa_ULLong
+#defineULong _PyDtoa_ULong
+#defineLong _PyDtoa_Long
+#defineULLong _PyDtoa_ULLong
+#defineKmax _PyDtoa_Kmax
+typedefstruct_PyDtoa_BigintBigint;

Uh oh!

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

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

Uh oh!

Conversation

JunyiXie commented Mar 11, 2021• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Mar 11, 2021

Uh oh!

JunyiXie commented Mar 12, 2021

Uh oh!

bedevere-bot commented Mar 12, 2021

Uh oh!

JunyiXie commented Mar 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JunyiXie commented Mar 12, 2021

Uh oh!

JunyiXie commented Mar 12, 2021

Uh oh!

bedevere-bot commented Mar 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JunyiXie commented Mar 12, 2021

Uh oh!

bedevere-bot commented Mar 12, 2021

Uh oh!

vstinner commented Mar 12, 2021

Uh oh!

JunyiXie commented Mar 13, 2021• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Mar 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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

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