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-112075: Accessing a single element should optimistically avoid locking by DinoV · Pull Request #115109 · python/cpython

Conversation

@DinoV

@DinoV DinoV commented Feb 6, 2024 •
edited
Loading

Uh oh!

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

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.

  • Issue: Make dict objects thread-safe in --disable-gil builds #112075

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot mentioned this pull request Feb 6, 2024
Make dict objects thread-safe in --disable-gil builds #112075
Closed
@DinoV DinoV added skip news topic-free-threading labels Feb 6, 2024
@DinoV
DinoV force-pushed the nogil_dict_get_threadsafe branch from 1266799 to 2f46a39 Compare February 6, 2024 22:49
@DinoV DinoV mentioned this pull request Feb 6, 2024
gh-112075: Iterating a dict shouldn't require locks #115108
Merged
@DinoV
DinoV force-pushed the nogil_dict_get_threadsafe branch 8 times, most recently from ef7c6ea to 54095de Compare February 7, 2024 20:31
@DinoV
DinoV requested a review from colesbury February 7, 2024 20:48
@DinoV
DinoV marked this pull request as ready for review February 7, 2024 20:58
@DinoV
DinoV requested review from markshannon and methane as code owners February 7, 2024 20:58
@bedevere-app bedevere-app Bot added the awaiting core review label Feb 7, 2024
colesbury
colesbury reviewed Feb 8, 2024
View reviewed changes
Comment thread Objects/dictobject.c Resolved

Uh oh!

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

Uh oh!

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

Uh oh!

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

Uh oh!

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

Uh oh!

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

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/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);

@colesbury colesbury Feb 8, 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.

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.

Sorry, something went wrong.

Uh oh!

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

@DinoV DinoV Feb 15, 2024

Copy link
Copy Markdown
Contributor Author

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
colesbury reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Objects/dictobject.c Outdated
if (hash == -1)
return NULL;
}
#ifdef Py_GIL_DISABLED

@colesbury colesbury Feb 8, 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.

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

Sorry, something went wrong.

Uh oh!

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

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/dictobject.c
PyObject *value;
Py_ssize_t ix;

#ifdef Py_GIL_DISABLED

@colesbury colesbury Feb 8, 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.

This also looks like contains_known_hash

Sorry, something went wrong.

Uh oh!

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

@DinoV DinoV Feb 16, 2024

Copy link
Copy Markdown
Contributor Author

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.

Or does contains_known_hash look like this? 🤔

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@DinoV
DinoV force-pushed the nogil_dict_get_threadsafe branch from 54095de to af7ddb9 Compare February 16, 2024 00:12
colesbury
colesbury approved these changes Feb 20, 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.

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Feb 20, 2024
DinoV added 2 commits February 20, 2024 16:43
@DinoV
Accessing a single element should optimistically avoid locking for pe…
6a584a2 
…rformance
@DinoV
Less duplication, improved comments, better handling of new keys duri…
d608fb3 
…ng resize
@DinoV
DinoV force-pushed the nogil_dict_get_threadsafe branch from af7ddb9 to d608fb3 Compare February 21, 2024 00:44
@DinoV
DinoV merged commit 5407146 into python:main Feb 21, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Feb 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 free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colesbury colesbury colesbury approved these changes
@methane methane Awaiting requested review from methane methane is a code owner
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

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.

2 participants

@DinoV @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.