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: Free-threaded dict odict basics by DinoV · Pull Request #114778 · python/cpython

Conversation

@DinoV

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

Uh oh!

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

Copy link
Copy Markdown
Contributor

This adds some basic locking to the ordered dict implementation. Nothing fancy, we just take some critical sections and expose _PyDict_SetItem_KnownHash_LockHeld internally. This will help make it easier to assert in the dictionary API that we have the proper locking in place.

  • 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 31, 2024
Make dict objects thread-safe in --disable-gil builds #112075
Closed
@DinoV DinoV added skip news topic-free-threading labels Jan 31, 2024
@DinoV
DinoV force-pushed the nogil_dict_odict_basics branch from 77e5821 to 7efcb0f Compare January 31, 2024 01:33
DinoV added 2 commits January 31, 2024 09:23
@DinoV
Include critical sections in dict API
3217169
@DinoV
OrderedDict thread safety
355a6ce
@DinoV
DinoV force-pushed the nogil_dict_odict_basics branch from 7efcb0f to 355a6ce Compare January 31, 2024 17:24
@DinoV
DinoV marked this pull request as ready for review January 31, 2024 18:28
@DinoV
DinoV requested review from markshannon and methane as code owners January 31, 2024 18:28
@bedevere-app bedevere-app Bot added the awaiting core review label Jan 31, 2024
@DinoV
DinoV requested review from colesbury and ericsnowcurrently and removed request for markshannon and methane January 31, 2024 18:28
colesbury
colesbury reviewed Jan 31, 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.

Some comments below, mostly about formatting.

I'd prefer we focus the thread-safety changes on dict and not OrderedDict for now. OrderedDict is both a bit of a mess in general and harder to do locking in a way that doesn't lead to re-acquiring the lock for the same object.

This will help make it easier to assert in the dictionary API that we have the proper locking in place

I'm a bit skeptical about these sorts of asserts in dict (or list). I expect there to be at least a few places internally where we want to call the unlocked APIs because we know it's safe for other reasons.

For example, the LRU caches uses _PyDict_SetItem_KnownHash, but we will want to do the locking on the lru_cache_object and not the internal dict, and can use the unlocked ("lock_held") variant.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Objects/odictobject.c Resolved

Uh oh!

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

Comment thread Include/internal/pycore_dict.h
// Export for '_asyncio' shared extension
PyAPI_FUNC(int) _PyDict_SetItem_KnownHash(PyObject *mp, PyObject *key,
PyObject *item, Py_hash_t hash);
int _PyDict_SetItem_KnownHash_LockHeld(PyObject *mp, PyObject *key,

@colesbury colesbury Jan 31, 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.

extern int for consistency with other non-exported functions in this header

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 Resolved

Uh oh!

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

Comment thread Objects/odictobject.c


/*[clinic input]
@critical_section

@colesbury colesbury Jan 31, 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 looks like it will call functions that themselves re-acquire the critical section.

Sorry, something went wrong.

Uh oh!

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

All reactions
@DinoV

DinoV commented Feb 1, 2024

Copy link
Copy Markdown
Contributor Author

Some comments below, mostly about formatting.

I'd prefer we focus the thread-safety changes on dict and not OrderedDict for now. OrderedDict is both a bit of a mess in general and harder to do locking in a way that doesn't lead to re-acquiring the lock for the same object.

This will help make it easier to assert in the dictionary API that we have the proper locking in place

I'm a bit skeptical about these sorts of asserts in dict (or list). I expect there to be at least a few places internally where we want to call the unlocked APIs because we know it's safe for other reasons.

For example, the LRU caches uses _PyDict_SetItem_KnownHash, but we will want to do the locking on the lru_cache_object and not the internal dict, and can use the unlocked ("lock_held") variant.

Really these changes are from a focus on dict thread safety! It's kind of hard to know that our implementation is actually thread safe without being able to make assertions on where things are being used. And getting a clean run isn't possible without fixing up some of these oddball cases (and I'm only trying to do the bare minimum here, hence why we do things like call back into locking APIs with the lock already held).

I think these asserts do have value - not only are they exposing these subtle external usages they also exposed the issue with dict_traverse being called from get_referrents. They also put a really bright line on the usage of APIs borrowing references, but for testing purposes I just made those lock and be broken (and obviously those are much more discoverable manually).

We don't necessarily need to land asserts but I think it'll eliminate a lot of hard to track down bugs by at least having done a pass with them in (which these 2 PRs + your get_referrants PR basically completes modulo the known borrowing APIs). It'd be nice if we could land them to prevent hard to track down bugs in the future though.

I think we could always make some accommodations for callers which need to call lock free (which might even be locking in a debug build, but that's probably less than ideal) but it'd be nice if the default behavior when you're trying to be smart is that you get a loud failure that shows you thought about it rather than just accidentally have done the wrong thing :)

All reactions

Sorry, something went wrong.

Uh oh!

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

@DinoV DinoV closed this Sep 27, 2024
@colesbury colesbury mentioned this pull request Oct 28, 2024
nogil segmentation fault on ordered dict operations #125996
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colesbury colesbury colesbury left review comments
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently

Uh oh!

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

Assignees

No one assigned

Labels

awaiting core review 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 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.