GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-112087: Use QSBR technique for list_new/clear for free-thread build by corona10 · Pull Request #115875 · python/cpython

Conversation

@corona10

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Make list objects thread-safe in --disable-gil builds #112087

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added the awaiting core review label Feb 24, 2024
@corona10 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 corona10 added skip news topic-free-threading labels Feb 24, 2024
@corona10 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 bedevere-app Bot mentioned this pull request Feb 24, 2024
Make list objects thread-safe in --disable-gil builds #112087
Closed
4 tasks done
@corona10
pythongh-112087: Use QSBR technique for list_new/clear for free-thread
556c016
@corona10
corona10 force-pushed the gh-115605-qsbr branch from de49474 to 556c016 Compare February 24, 2024 06:48
@corona10 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
corona10 commented Feb 24, 2024
View reviewed changes
Comment thread Lib/test/test_list.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@corona10
corona10 requested a review from colesbury February 24, 2024 07:01
@corona10 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
@corona10
Replace list_preallocate_exact to list_ensure_capacity
67a7f32
@corona10
fix
6ad0ab4
@corona10
Update test
6264094
@corona10

corona10 commented Feb 24, 2024

Copy link
Copy Markdown
Member Author 
➜  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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10
Update list_item
e514c65
colesbury
colesbury reviewed Feb 28, 2024
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
corona10 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Objects/listobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/listobject.c
{
PyListObject *a = (PyListObject *)aa;
PyObject *item = NULL;
Py_BEGIN_CRITICAL_SECTION(a);

@colesbury colesbury Feb 28, 2024

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/listobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/listobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/listobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
corona10 added 3 commits February 29, 2024 07:03
@corona10
Revert memory optimization changes
df90d37
@corona10
Address code review
e7f3683
@corona10
Merge remote-tracking branch 'upstream/main' into pythongh-115605-qsbr
0eb3657
@corona10
corona10 requested a review from colesbury February 28, 2024 22:24
colesbury
colesbury reviewed Feb 29, 2024
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/listobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/listobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@corona10
Revert list_prealloc
41204cf
@corona10

corona10 commented Mar 1, 2024

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10
corona10 requested a review from colesbury March 1, 2024 01:24
@corona10
nit
9f5d048
colesbury
colesbury approved these changes Mar 1, 2024
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Mar 1, 2024
@corona10
corona10 merged commit fb5e034 into python:main Mar 1, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Mar 1, 2024
@corona10
corona10 deleted the gh-115605-qsbr branch March 1, 2024 23:30
@bedevere-bot

bedevere-bot commented Mar 2, 2024

Copy link
Copy Markdown

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

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

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. 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.
  4. Check if the failure is related to this commit (fb5e034) or if it is a false positive.
  5. 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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10

corona10 commented Mar 2, 2024

Copy link
Copy Markdown
Member Author
  • 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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10

corona10 commented Mar 2, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10

corona10 commented Mar 2, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10

corona10 commented Mar 2, 2024

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10 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
@corona10 @woodruffw
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
@corona10 @adorilson
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
@corona10 @diegorusso
pythongh-112087: Use QSBR technique for list_new/clear for free-threa…
664d810 
…d build (pythongh-115875)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colesbury colesbury colesbury approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news topic-free-threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@corona10 @bedevere-bot @colesbury
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.