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: Update list_get_item_ref to optimistically avoid locking#116353
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 Mar 5, 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.
corona10 commented Mar 5, 2024
!buildbot nogil |
bedevere-bot commented Mar 5, 2024
🤖 New build scheduled with the buildbot fleet by @corona10 for commit eda29bc 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
corona10 commented Mar 5, 2024
!buildbot nogil |
bedevere-bot commented Mar 5, 2024
🤖 New build scheduled with the buildbot fleet by @corona10 for commit 9071dd4 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
corona10 commented Mar 5, 2024
IIUC, This PR should be the last PR for #112087 |
Uh oh!
There was an error while loading. Please reload this page.
| if (ob_item==NULL){ | ||
| returnNULL; | ||
| } | ||
| Py_ssize_tcap=_Py_atomic_load_ssize_relaxed(&op->allocated); |
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.
This is okay for now, but we need to store the capacity at the start of the ob_item allocation in the free-threaded build.
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 may need to create a issue for tracking this.
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.
It's necessary for thread-safety so I think it's covered by #112087
corona10Mar 5, 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.
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.
By the way, we already store the op->allocated at the ob_item allocation.
So adding assertion will be enough?assert(cap != -1 && cap >= size);
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.
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 currently store it in the PyListObject. We also need to store it in the same memory allocation as the ob_item array, like a pre-header.
Here are some pointers to the relevant code in nogil-3.12:
- https://github.com/colesbury/nogil-3.12/blob/cedde4f5ec3759ad723c89d44738776f362df564/Include/cpython/listobject.h#L24-L27
- https://github.com/colesbury/nogil-3.12/blob/cedde4f5ec3759ad723c89d44738776f362df564/Objects/listobject.c#L65-L70
The problem with the current code is that the list may be resized concurrently with the access. The bounds check may be stale. Putting the value of "allocated" as an immutable field avoids this problem.
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.
Ah, I got it. I understood what you want to say. Let's handle it at a separate PR.
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.
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.
Co-authored-by: Sam Gross <colesbury@gmail.com>
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.
This looks good to me. I think it's best to address the capacity/allocated changes in a separate PR.
…king (pythongh-116353) Co-authored-by: Sam Gross <colesbury@gmail.com>
…king (pythongh-116353) Co-authored-by: Sam Gross <colesbury@gmail.com>
listobjects thread-safe in--disable-gilbuilds #112087