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: _Py_dict_lookup needs to lock shared keys by DinoV · Pull Request #117528 · python/cpython

Conversation

@DinoV

@DinoV DinoV commented Apr 3, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

_Py_dict_lookup needs to lock the shared keys if we have a split dictionary. If we're looking up with a non-exact unicode we need to also incref the keys as the lookup could mutate the keys and we could lose the last reference.

insertdict is updated to avoid contention on the shared dict lookup by calling the threadsafe unicode lookup directly and only falling back to _Py_dict_lookup if the thread safe lookup can't succeed.

  • 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 Apr 3, 2024
Make dict objects thread-safe in --disable-gil builds #112075
Closed
@DinoV
DinoV requested a review from colesbury April 3, 2024 23:53
@DinoV DinoV added skip news topic-free-threading labels Apr 3, 2024
@DinoV
DinoV force-pushed the nogil_dict_shared_key_insert branch 2 times, most recently from c51437d to f3e3db3 Compare April 4, 2024 00:04
@DinoV
DinoV marked this pull request as ready for review April 4, 2024 00:35
@DinoV
DinoV requested review from markshannon and methane as code owners April 4, 2024 00:35
@bedevere-app bedevere-app Bot added the awaiting core review label Apr 4, 2024
colesbury
colesbury reviewed Apr 4, 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 looks right, but I'm still seeing issues with the test that uncovered the problem. I'll investigate more today.

Sorry, something went wrong.

Uh oh!

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

Uh oh!

There was an error while loading. Please reload this page.
@colesbury colesbury mentioned this pull request Apr 4, 2024
Disable GIL by default in free-threaded build #116749
Closed
colesbury
colesbury reviewed Apr 4, 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_shared_key_insert branch from 8942e4d to 2f5f5a1 Compare April 5, 2024 02:23
@colesbury colesbury mentioned this pull request Apr 12, 2024
gh-112075: Lock when inserting into shared keys #117824
Closed
@DinoV
DinoV force-pushed the nogil_dict_shared_key_insert branch 10 times, most recently from 35cf56e to d580aa5 Compare April 16, 2024 01:31
colesbury
colesbury reviewed Apr 16, 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.

I'm still looking at _Py_dict_lookup, but I think insertdict and dict_setdefault_ref_lock_held can be simpler if they handle split dictionaries early on and separate from the main code path.

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.
@DinoV
DinoV force-pushed the nogil_dict_shared_key_insert branch 2 times, most recently from cfd659c to 19d1446 Compare April 24, 2024 20:30
DinoV added 2 commits April 25, 2024 09:17
@DinoV
Lock shared keys in Py_dict_lookup and use thread-safe lookup in `i…
7e75eb2 
…nsertdict`
@DinoV
Assert we always have unicode only key in split dict case
5f498f7
DinoV and others added 5 commits April 25, 2024 09:17
@DinoV
Re-probe for index with shared keys locked before adding split index
90fdea3
@DinoV
Refactor to avoid potential to lock keys twice, share impl between in…
1272d68 
…sertdict and setdefault
@colesbury @DinoV
pythongh-112075: Lock when inserting into shared keys
002ca04
@colesbury @DinoV
Fix return type
e7d7430
@DinoV
Remove splitdict_lookup_keys_lock_held
a74c0ee
@DinoV
DinoV force-pushed the nogil_dict_shared_key_insert branch from 19d1446 to a74c0ee Compare April 25, 2024 16:19
colesbury
colesbury approved these changes Apr 25, 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 other than the unused code warning in the default build

Sorry, something went wrong.

Uh oh!

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

static Py_ssize_t
unicodekeys_lookup_unicode_threadsafe(PyDictKeysObject* dk, PyObject *key,

@colesbury colesbury Apr 25, 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.

Unused code warning

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
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Apr 25, 2024
@DinoV
unicodekeys_lookup_unicode_threadsafe only when gil is disabled
d5a5403
@DinoV
DinoV merged commit d5df252 into python:main Apr 25, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Apr 25, 2024
@DinoV
DinoV deleted the nogil_dict_shared_key_insert branch May 31, 2024 18:22
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.