Skip to content

gh-112075: Accessing a single element should optimistically avoid locking - #115109

Merged
DinoV merged 2 commits into
python:mainfrom
DinoV:nogil_dict_get_threadsafe
Feb 21, 2024
Merged

gh-112075: Accessing a single element should optimistically avoid locking#115109
DinoV merged 2 commits into
python:mainfrom
DinoV:nogil_dict_get_threadsafe

Conversation

@DinoV

@DinoVDinoV commented Feb 6, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

Makes accessing a single element thread safe.

Adds tracking for whether a dictionary is shared or not, but some of this gets duplicated in #115108.

Does not yet deal with all of the atomic assignments that need to happen to make this 100% right, that'll come in a separate PR.

@bedevere-appbedevere-appBot mentioned this pull request Feb 6, 2024
Closed
@DinoV
DinoVforce-pushed the nogil_dict_get_threadsafe branch from 1266799 to 2f46a39CompareFebruary 6, 2024 22:49
@DinoVDinoV mentioned this pull request Feb 6, 2024
Merged
@DinoV
DinoVforce-pushed the nogil_dict_get_threadsafe branch 8 times, most recently from ef7c6ea to 54095deCompareFebruary 7, 2024 20:31
@DinoV
DinoV requested a review from colesburyFebruary 7, 2024 20:48
@DinoV
DinoV marked this pull request as ready for review February 7, 2024 20:58
colesbury
colesbury reviewed Feb 8, 2024
View reviewed changes
Comment threadObjects/dictobject.c Outdated
Comment threadObjects/dictobject.c Outdated
Comment threadObjects/dictobject.c Outdated
Comment threadObjects/dictobject.c Outdated
Comment threadObjects/dictobject.c Outdated
Comment threadObjects/dictobject.c Outdated
Comment threadObjects/dictobject.c Outdated
Comment on lines +1792 to +1794
set_keys(mp, new_keys_object(interp, log2_newsize, unicode));
if (mp->ma_keys == NULL) {
mp->ma_keys = oldkeys;
set_keys(mp, oldkeys);

@colesburycolesburyFeb 8, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think the possible temporary assignment to NULL is thread-safe without the GIL. Let's assign the result of new_keys_object() to a temporary variable and check that before assigning it to mp->ma_keys.

@DinoVDinoVFeb 15, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

When I was looking at this function when you made a comment about it on #114741 I realized that the assignment of it to a new empty table isn't right at all! We really need to allocate the new table, copy everything over, and then we can publish it.

Comment threadObjects/dictobject.c Outdated
if (hash == -1)
return NULL;
}
#ifdef Py_GIL_DISABLED

@colesburycolesburyFeb 8, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this differ from PyDict_Contains()? Can we just call that?

Comment threadObjects/dictobject.c Outdated
Comment threadObjects/dictobject.c
PyObject *value;
Py_ssize_t ix;

#ifdef Py_GIL_DISABLED

@colesburycolesburyFeb 8, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This also looks like contains_known_hash

@DinoVDinoVFeb 16, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Or does contains_known_hash look like this? 🤔

@DinoV
DinoVforce-pushed the nogil_dict_get_threadsafe branch from 54095de to af7ddb9CompareFebruary 16, 2024 00:12
colesbury
colesbury approved these changes Feb 20, 2024
View reviewed changes

@colesburycolesbury left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@DinoV
DinoVforce-pushed the nogil_dict_get_threadsafe branch from af7ddb9 to d608fb3CompareFebruary 21, 2024 00:44
@DinoV
DinoV merged commit 5407146 into python:mainFeb 21, 2024
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
@DinoV@woodruffw
pythongh-112075: Accessing a single element should optimistically avo…
16d801f
…id locking (python#115109)
Makes accessing a single element thread safe and typically lock free
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@DinoV@diegorusso
pythongh-112075: Accessing a single element should optimistically avo…
beeaa4a
…id locking (python#115109)
Makes accessing a single element thread safe and typically lock free
@DinoV
DinoV deleted the nogil_dict_get_threadsafe branch May 31, 2024 18:23
LukasWoodtli pushed a commit to LukasWoodtli/cpython that referenced this pull request Jan 22, 2025
@DinoV@LukasWoodtli
pythongh-112075: Accessing a single element should optimistically avo…
de24ce3
…id locking (python#115109)
Makes accessing a single element thread safe and typically lock free
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colesburycolesburycolesbury approved these changes
@methanemethaneAwaiting requested review from methanemethane is a code owner
@markshannonmarkshannonAwaiting requested review from markshannonmarkshannon is a code owner

Assignees

No one assigned

Labels

skip newstopic-free-threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@DinoV@colesbury