Skip to content

gh-112075: Free-threaded dict odict basics #114778

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

DinoV
Copy link
Contributor

@DinoV DinoV commented Jan 31, 2024

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.

@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 ericsnowcurrently and colesbury and removed request for methane and markshannon January 31, 2024 18:28
Copy link
Contributor

@colesbury colesbury left a 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.

@@ -22,6 +22,9 @@ extern int _PyDict_DelItemIf(PyObject *mp, PyObject *key,
// 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,
Copy link
Contributor

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

@@ -984,6 +985,7 @@ odict_reduce(register PyODictObject *od, PyObject *Py_UNUSED(ignored))


/*[clinic input]
@critical_section
Copy link
Contributor

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.

@DinoV
Copy link
Contributor Author

DinoV commented Feb 1, 2024

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants