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

corona10 · 2024-02-24T06:46:49Z

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

Lib/test/test_list.py

corona10 · 2024-02-24T08:01:37Z

➜  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

colesbury

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.

Objects/listobject.c

colesbury · 2024-02-28T18:48:36Z

Objects/listobject.c

@@ -498,11 +597,21 @@ static PyObject *
 list_item(PyObject *aa, Py_ssize_t i)
 {
    PyListObject *a = (PyListObject *)aa;
+    PyObject *item = NULL;
+    Py_BEGIN_CRITICAL_SECTION(a);


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

Objects/listobject.c

colesbury

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.

Objects/listobject.c

corona10 · 2024-03-01T01:23:58Z

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 :)

bedevere-bot · 2024-03-02T03:59:14Z

⚠️⚠️⚠️ 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

corona10 · 2024-03-02T06:14:46Z

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

corona10 · 2024-03-02T06:51:59Z

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?

corona10 · 2024-03-02T07:07:24Z

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

corona10 · 2024-03-02T07:37:33Z

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

Lib/test/test_list.py Outdated Show resolved Hide resolved

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

Objects/listobject.c Outdated Show resolved Hide resolved

Objects/listobject.c Outdated Show resolved Hide resolved

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
34 checks passed

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)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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 #115875

corona10 commented Feb 24, 2024 •

edited by bedevere-app bot

Loading

corona10 commented Feb 24, 2024

colesbury left a comment

colesbury Feb 28, 2024

colesbury left a comment

corona10 commented Mar 1, 2024

bedevere-bot commented Mar 2, 2024

corona10 commented Mar 2, 2024

corona10 commented Mar 2, 2024 •

edited

Loading

corona10 commented Mar 2, 2024 •

edited

Loading

corona10 commented Mar 2, 2024

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 #115875

Conversation

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

corona10 commented Feb 24, 2024

colesbury left a comment

Choose a reason for hiding this comment

colesbury Feb 28, 2024

Choose a reason for hiding this comment

colesbury left a comment

Choose a reason for hiding this comment

corona10 commented Mar 1, 2024

bedevere-bot commented Mar 2, 2024

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

corona10 commented Mar 2, 2024

corona10 commented Mar 2, 2024 • edited Loading

corona10 commented Mar 2, 2024 • edited Loading

corona10 commented Mar 2, 2024

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