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: Make PyDictKeysObject thread-safe by DinoV · Pull Request #114741 · python/cpython

Conversation

@DinoV

@DinoV DinoV commented Jan 30, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Adds locking for shared keys

  • 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 Jan 30, 2024
Make dict objects thread-safe in --disable-gil builds #112075
Closed
@DinoV DinoV added skip news topic-free-threading labels Jan 30, 2024
@DinoV
DinoV force-pushed the nogil_dict_pydictkeys branch 3 times, most recently from fc6beda to 137eebe Compare January 30, 2024 01:19
corona10
corona10 reviewed Jan 30, 2024
View reviewed changes
Comment thread Objects/dictobject.c Outdated
_Py_DecRefTotal(_PyInterpreterState_GET());
#endif
if (--dk->dk_refcnt == 0) {
if (_Py_atomic_add_ssize(&dk->dk_refcnt, -1) == 1) {

@corona10 corona10 Jan 30, 2024

Copy link
Copy Markdown
Member

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.

How about adding a macro for the free-threading version and the default version?

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

@corona10 corona10 Jan 30, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

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 did similar approach at listobject: 393cbef

See: _Py_SET_ITEMREF

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland erlend-aasland Feb 2, 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.

@corona10 is there a significant difference in performance between the two builds? I'm not sure if it is worth it to add these preprocessor guards everywhere if there is no measurable effect :) Is there a different motivation than performance?

Sorry, something went wrong.

Uh oh!

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

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

Regardless of whether we do the macro approach everywhere, I think it's a good idea in this specific case (specifically for performance).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@DinoV
DinoV force-pushed the nogil_dict_pydictkeys branch from 137eebe to 0891529 Compare January 30, 2024 17:31
@DinoV
DinoV marked this pull request as ready for review January 30, 2024 18:05
@DinoV
DinoV requested review from markshannon and methane as code owners January 30, 2024 18:05
@bedevere-app bedevere-app Bot added the awaiting core review label Jan 30, 2024
methane
methane reviewed Jan 31, 2024
View reviewed changes
Comment thread Objects/dictobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@DinoV
DinoV force-pushed the nogil_dict_pydictkeys branch from 0891529 to 398cb23 Compare January 31, 2024 17:04
@colesbury
colesbury self-requested a review January 31, 2024 20:15
colesbury
colesbury reviewed Feb 1, 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.

This mostly looks good to me.

I don't think we want to be locking around shared_keys_usable_size(). It's both not sufficient for thread-safety at most of the call sites and not what we want to be doing for performance reasons.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
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.
@corona10 corona10 mentioned this pull request Feb 2, 2024
gh-112050: Make collections.deque thread-safe in free-threaded builds #113830
Merged
@DinoV
DinoV force-pushed the nogil_dict_pydictkeys branch 4 times, most recently from fe12c49 to 87c3446 Compare February 6, 2024 23:33
colesbury
colesbury reviewed Feb 7, 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 Outdated
Comment on lines +170 to +171
_Py_atomic_store_ssize(&keys->dk_nentries, keys->dk_nentries + 1);
_Py_atomic_store_ssize(&keys->dk_usable, keys->dk_usable - 1);

@colesbury colesbury Feb 7, 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 can use a weaker ordering here that will be faster, especially on x86 where "release" doesn't require any memory barrier:

    _Py_atomic_store_ssize_relaxed(&keys->dk_nentries, keys->dk_nentries + 1);
    _Py_atomic_store_ssize_release(&keys->dk_usable, keys->dk_usable - 1);

(I don't think we have _Py_atomic_store_ssize_release yet, though)

I find the memory orderings hard to reason correctly about, so I like to model them with CDSChecker. Here's the model I used for this:
https://github.com/colesbury/c11-model-checker/blob/cpython-models/test/gh-112075.c

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
DinoV reacted with heart 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 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.
colesbury
colesbury reviewed Feb 8, 2024
View reviewed changes
Comment thread Include/cpython/pyatomic_msc.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Include/cpython/pyatomic_msc.h 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.
@DinoV
DinoV force-pushed the nogil_dict_pydictkeys branch from a22f723 to 808dd89 Compare February 15, 2024 23:45
@DinoV DinoV mentioned this pull request Feb 15, 2024
gh-112075: Accessing a single element should optimistically avoid locking #115109
Merged
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.

One minor formatting comment, but otherwise LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/dictobject.c Outdated
if (_PyDict_HasSplitTable(mp)) {
LOCK_KEYS(keys);
dictkeys_incref(keys);
} else {

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

Suggested change
} else {
}
else {

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 11:58
@DinoV
Make PyDictKeysObject thread-safe
4715734
@DinoV
Fix naming of unsplit -> combined
18d2916
@DinoV
DinoV force-pushed the nogil_dict_pydictkeys branch from 808dd89 to 4914ba8 Compare February 20, 2024 20:01
DinoV added 3 commits February 20, 2024 12:28
@DinoV
Lock less and deal with atomic updates to shared size
61caf84
@DinoV
Address review feedback
268c806
@DinoV
Lock in dictresize, not in callers
7407ce9
@DinoV
DinoV force-pushed the nogil_dict_pydictkeys branch 2 times, most recently from 90bea6a to 88ab576 Compare February 20, 2024 23:25
@DinoV
Fixup recursive locking
a9d3666
@DinoV
DinoV force-pushed the nogil_dict_pydictkeys branch from 88ab576 to a9d3666 Compare February 21, 2024 00:12
@DinoV
DinoV merged commit 176df09 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: Make PyDictKeysObject thread-safe (python#114741)
d577f63 
Adds locking for shared PyDictKeysObject's for dictionaries
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@DinoV @diegorusso
pythongh-112075: Make PyDictKeysObject thread-safe (python#114741)
97f461b 
Adds locking for shared PyDictKeysObject's for dictionaries
@DinoV
DinoV deleted the nogil_dict_pydictkeys 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: Make PyDictKeysObject thread-safe (python#114741)
eed7e93 
Adds locking for shared PyDictKeysObject's for dictionaries
@dpdani dpdani mentioned this pull request Jun 17, 2026
test_abc hangs on TSan Parallel Test on Free Threading #151593
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@methane methane methane left review comments
@corona10 corona10 corona10 left review comments
@erlend-aasland erlend-aasland erlend-aasland left review comments
@colesbury colesbury colesbury approved these changes
@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.

5 participants

@DinoV @methane @colesbury @corona10 @erlend-aasland
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 01402520 — 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.