gh-112087: Use QSBR technique for list_new/clear for free-thread build by corona10 · Pull Request #115875 · python/cpython · GitHub

Issue: Make list objects thread-safe in --disable-gil builds #112087

➜  cpython git:(gh-115605-qsbr) ✗ ./python.exe -m test test_list test_iter test_sys -R 3:3
Using random seed: 3402256694
Raised RLIMIT_NOFILE: 256 -> 1024
0:00:00 load avg: 2.17 Run 3 tests sequentially
0:00:00 load avg: 2.17 [1/3] test_list
beginning 6 repetitions
123456
......
0:00:01 load avg: 2.24 [2/3] test_iter
beginning 6 repetitions
123456
......
0:00:01 load avg: 2.24 [3/3] test_sys
beginning 6 repetitions
123456
......

== Tests result: SUCCESS ==

All 3 tests OK.

Total duration: 6.7 sec
Total tests: run=189 skipped=8
Total test files: run=3/3
Result: SUCCESS

Thanks for taking this on, and sorry for the delay in reviewing this.

The list_clear change looks good, but I found the other parts difficult to review because the PR includes changes that aren't necessary for using QSBR. In particular:

The list_good_size is a memory optimization so that requested sizes match mimalloc's internal size classes, but it's not necessary for thread-safety. Additionally, the code assumes that the allocated size is stored in the allocated ob_item array, but that's not implemented yet, so it's hard to tell if the code is correct.
Similarly, list_ensure_capacity was written to simplify some resize checks given that we will generally want to match mimalloc's size classes rather than do the exact size allocations, but it's not necessary for correctness and not used here as it was used in the nogil-3.12 fork, which makes it harder to review.

I think it'll be easier to review (and maintain) to start with the narrow thread-safety changes and tackle the memory optimizations later.

We will want the fast path to do this without locking soon.

What is the motivation behind the refactoring of list_new_prealloc and PyList_New()? It doesn't appear to be necessary for thread-safety. I'm a bit concerned that the change to list_new_prealloc to zero initialize ob_item via PyMem_Calloc may have a performance impact on the default build.

What is the motivation behind the refactoring of list_new_prealloc and PyList_New()? I

Ah, it was the legacy of my first patch, including mimalloc-based optimization.
I revert them all :)

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot ARM64 MacOS M1 Refleaks NoGIL 3.x has failed when building commit fb5e034.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/1368/builds/377) and take a look at the build logs.
Check if the failure is related to this commit (fb5e034) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/1368/builds/377

Failed tests:

test.test_concurrent_futures.test_shutdown
test_sched

Test leaking resources:

test_sched: memory blocks
test_shutdown: memory blocks

Summary of the results of the build (if available):

==

Click to see traceback logs

Note: switching to 'fb5e0344e41788988171f31c6b8d4fd1a13b9041'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at fb5e0344e4 gh-112087: Use QSBR technique for list_new/clear for free-thread build (gh-115875)
Switched to and reset branch 'main'

In file included from ./Modules/tkappinit.c:17:
In file included from /opt/homebrew/Cellar/tcl-tk/8.6.13_5/include/tcl-tk/tk.h:99:
/opt/homebrew/Cellar/tcl-tk/8.6.13_5/include/tcl-tk/X11/Xlib.h:131:21: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
        int (*free_private)();  /* called to free private storage */
                           ^
                            void
In file included from ./Modules/_tkinter.c:52:
In file included from /opt/homebrew/Cellar/tcl-tk/8.6.13_5/include/tcl-tk/tk.h:99:
/opt/homebrew/Cellar/tcl-tk/8.6.13_5/include/tcl-tk/X11/Xlib.h:131:21: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
        int (*free_private)();  /* called to free private storage */
                           ^
                            void
/opt/homebrew/Cellar/tcl-tk/8.6.13_5/include/tcl-tk/X11/Xlib.h:334:33: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
        struct _XImage *(*create_image)();
                                       ^
                                        void
/opt/homebrew/Cellar/tcl-tk/8.6.13_5/include/tcl-tk/X11/Xlib.h:334:33: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
        struct _XImage *(*create_image)();
                                       ^
                                        void
/opt/homebrew/Cellar/tcl-tk/8.6.13_5/include/tcl-tk/X11/Xlib.h:453:23: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
        XID (*resource_alloc)(); /* allocator function */
                             ^
                              void
/opt/homebrew/Cellar/tcl-tk/8.6.13_5/include/tcl-tk/X11/Xlib.h:471:20: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
        int (*synchandler)();   /* Synchronization handler */
                          ^
                           void
/opt/homebrew/Cellar/tcl-tk/8.6.13_5/include/tcl-tk/X11/Xlib.h:496:24: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
        Bool (*event_vec[128])();  /* vector for wire to event */
                              ^
                               void
/opt/homebrew/Cellar/tcl-tk/8.6.13_5/include/tcl-tk/X11/Xlib.h/opt/homebrew/Cellar/tcl-tk/8.6.13_5/include/tcl-tk/X11/Xlib.h::453497::2325::  warning: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]

        XID (*resource_alloc)(); /* allocator function */        Status (*wire_vec[128])(); /* vector for event to wire */

                             ^                               ^

                              void                                void

/opt/homebrew/Cellar/tcl-tk/8.6.13_5/include/tcl-tk/X11/Xlib.h:509:20: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
        Bool (**error_vec)();      /* vector for wire to error *//opt/homebrew/Cellar/tcl-tk/8.6.13_5/include/tcl-tk/X11/Xlib.h
:                          ^471
:                           void20
: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
        int (*synchandler)();   /* Synchronization handler */
                          ^
                           void
/opt/homebrew/Cellar/tcl-tk/8.6.13_5/include/tcl-tk/X11/Xlib.h:522:25: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
        int (*savedsynchandler)(); /* user synchandler when Xlib usurps */
                               ^
                                void
/opt/homebrew/Cellar/tcl-tk/8.6.13_5/include/tcl-tk/X11/Xlib.h:496:24: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
        Bool (*event_vec[128])();  /* vector for wire to event */
                              ^
                               void
/opt/homebrew/Cellar/tcl-tk/8.6.13_5/include/tcl-tk/X11/Xlib.h:497:25: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
        Status (*wire_vec[128])(); /* vector for event to wire */
                               ^
                                void
/opt/homebrew/Cellar/tcl-tk/8.6.13_5/include/tcl-tk/X11/Xlib.h:509:20: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
        Bool (**error_vec)();      /* vector for wire to error */
                          ^
                           void
/opt/homebrew/Cellar/tcl-tk/8.6.13_5/include/tcl-tk/X11/Xlib.h:522:25: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
        int (*savedsynchandler)(); /* user synchandler when Xlib usurps */
                               ^
                                void
/opt/homebrew/Cellar/tcl-tk/8.6.13_5/include/tcl-tk/X11/Xlib.h:1053:24: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
typedef void (*XIMProc)();
                       ^
                        void
/opt/homebrew/Cellar/tcl-tk/8.6.13_5/include/tcl-tk/X11/Xlib.h:1053:24: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
typedef void (*XIMProc)();
                       ^
                        void
9 warnings generated.
9 warnings generated.

make: *** [buildbottest] Error 2

test_sched: Already leaked but increased -> test_sched leaked [1, 0, 0] memory blocks, sum=1 (this is fine) to test_sched leaked [5, 4, 4] memory blocks, sum=13
test.test_concurrent_futures.test_shutdown looks fine in my loacl.

Using random seed: 2687821224
Raised RLIMIT_NOFILE: 256 -> 1024
0:00:00 load avg: 2.41 Run 1 test sequentially
0:00:00 load avg: 2.41 [1/1] test_concurrent_futures.test_shutdown
beginning 6 repetitions. Showing number of leaks (. for 0 or less, X for 10 or more)
123:456
XX1 ...
test_concurrent_futures.test_shutdown passed in 1 min 13 sec

== Tests result: SUCCESS ==

1 test OK.

Total duration: 1 min 13 sec
Total tests: run=47 skipped=3
Total test files: run=1/1
Result: SUCCESS

Root cause: _PyMem_ProcessDelayed is not executed

cpython/Objects/obmalloc.c

Lines 1021 to 1023 in 5dc8c84

    
           if (buf->wr_idx == WORK_ITEMS_PER_CHUNK) { 
        
               _PyMem_ProcessDelayed((PyThreadState *)tstate); 
        
           }

@colesbury Is there a way to trigger _PyMem_ProcessDelayed by force?

With #116237 (Not sure that it will be the proper solution)

➜  cpython git:(gh-112087-leak-tmp) ✗ ./python.exe -m test test_sched -R 3:3
Using random seed: 2623432415
Raised RLIMIT_NOFILE: 256 -> 1024
0:00:00 load avg: 2.50 Run 1 test sequentially
0:00:00 load avg: 2.50 [1/1] test_sched
beginning 6 repetitions. Showing number of leaks (. for 0 or less, X for 10 or more)
123:456
XX4 5.4
test_sched leaked [0, 0, 1] references, sum=1 (this is fine)
test_sched leaked [5, -16, 4] memory blocks, sum=-7 (this is fine)

== Tests result: SUCCESS ==

1 test OK.

Total duration: 3.4 sec
Total tests: run=11
Total test files: run=1/1
Result: SUCCESS

Okay #116237 fix the issue, but I prefer #116238

…d build (pythongh-115875)

bedevere-app Bot added the awaiting core review label Feb 24, 2024

corona10 changed the title gh-115605: Use QSBR technique for list_new/free gh-115605: Use QSBR technique for list_new/clear Feb 24, 2024

corona10 added skip news topic-free-threading labels Feb 24, 2024

corona10 changed the title gh-115605: Use QSBR technique for list_new/clear gh-112087: Use QSBR technique for list_new/clear Feb 24, 2024

bedevere-app Bot mentioned this pull request Feb 24, 2024

Make list objects thread-safe in --disable-gil builds #112087

Closed

4 tasks

pythongh-112087 : Use QSBR technique for list_new/clear for free-thread

556c016

corona10 force-pushed the gh-115605-qsbr branch from de49474 to 556c016 Compare February 24, 2024 06:48

corona10 changed the title gh-112087: Use QSBR technique for list_new/clear gh-112087: Use QSBR technique for list_new/clear for free-thread build Feb 24, 2024

corona10 commented Feb 24, 2024

View reviewed changes

Comment thread Lib/test/test_list.py Outdated

corona10 requested a review from colesbury February 24, 2024 07:01

corona10 mentioned this pull request Feb 24, 2024

gh-112087: Make list_{subscript, ass_slice, slice} to be thread-safe #115854

Closed

corona10 added 3 commits February 24, 2024 16:45

Replace list_preallocate_exact to list_ensure_capacity

67a7f32

fix

6ad0ab4

Update test

6264094

Update list_item

e514c65

colesbury reviewed Feb 28, 2024

View reviewed changes

corona10 added 3 commits February 29, 2024 07:03

Revert memory optimization changes

df90d37

Address code review

e7f3683

Merge remote-tracking branch 'upstream/main' into pythongh-115605 -qsbr

0eb3657

corona10 requested a review from colesbury February 28, 2024 22:24

colesbury reviewed Feb 29, 2024

View reviewed changes

Comment thread Objects/listobject.c Outdated

Comment thread Objects/listobject.c Outdated

Revert list_prealloc

41204cf

corona10 requested a review from colesbury March 1, 2024 01:24

nit

9f5d048

colesbury approved these changes Mar 1, 2024

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting core review labels Mar 1, 2024

corona10 merged commit fb5e034 into python:main Mar 1, 2024

bedevere-app Bot removed the awaiting merge label Mar 1, 2024

corona10 deleted the gh-115605-qsbr branch March 1, 2024 23:30

corona10 mentioned this pull request Mar 2, 2024

gh-115103: Update gc.collect to process delayed objects #116238

Merged

woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024

pythongh-112087 : Use QSBR technique for list_new/clear for free-threa…

93a1af4

…d build (pythongh-115875)

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024

pythongh-112087 : Use QSBR technique for list_new/clear for free-threa…

3106ad4

…d build (pythongh-115875)

diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024

pythongh-112087 : Use QSBR technique for list_new/clear for free-threa…

664d810

…d build (pythongh-115875)

               {
                   PyListObject *a = (PyListObject *)aa;
+                  PyObject *item = NULL;
+                  Py_BEGIN_CRITICAL_SECTION(a);

gh-112087: Use QSBR technique for list_new/clear for free-thread build by corona10 · Pull Request #115875 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-112087: Use QSBR technique for list_new/clear for free-thread build - #115875

gh-112087: Use QSBR technique for list_new/clear for free-thread build#115875corona10 merged 10 commits intopython:mainpython/cpython:mainfrom corona10:gh-115605-qsbrcorona10/cpython:gh-115605-qsbrCopy head branch name to clipboard

Conversation

corona10 commented Feb 24, 2024 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

corona10 commented Feb 24, 2024

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

colesbury Feb 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

corona10 commented Mar 1, 2024

Uh oh!

bedevere-bot commented Mar 2, 2024

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

corona10 commented Mar 2, 2024

Uh oh!

corona10 commented Mar 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corona10 commented Mar 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corona10 commented Mar 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gh-112087: Use QSBR technique for list_new/clear for free-thread build#115875
corona10 merged 10 commits into
python:mainfrom
corona10:gh-115605-qsbr

corona10 commented Feb 24, 2024 •
edited by bedevere-app Bot
Loading

corona10 commented Mar 2, 2024 •
edited
Loading

corona10 commented Mar 2, 2024 •
edited
Loading