Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-112087: Use QSBR technique for list_new/clear for free-thread build#115875
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Conversation
corona10 commented Feb 24, 2024 • edited by bedevere-app bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-app bot
Uh oh!
There was an error while loading. Please reload this page.
de49474 to 556c016CompareUh oh!
There was an error while loading. Please reload this page.
corona10 commented Feb 24, 2024
|
colesbury left a comment
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_sizeis 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 allocatedob_itemarray, but that's not implemented yet, so it's hard to tell if the code is correct. - Similarly,
list_ensure_capacitywas 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.
Uh oh!
There was an error while loading. Please reload this page.
| { | ||
| PyListObject*a= (PyListObject*)aa; | ||
| PyObject*item=NULL; | ||
| Py_BEGIN_CRITICAL_SECTION(a); |
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
colesbury left a comment
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
corona10 commented Mar 1, 2024
Ah, it was the legacy of my first patch, including mimalloc-based optimization. |
bedevere-bot commented Mar 2, 2024
|
corona10 commented Mar 2, 2024
|
corona10 commented Mar 2, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Root cause: Lines 1021 to 1023 in 5dc8c84
@colesbury Is there a way to trigger _PyMem_ProcessDelayed by force? |
corona10 commented Mar 2, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
With #116237 (Not sure that it will be the proper solution) |
listobjects thread-safe in--disable-gilbuilds #112087