gh-112075: Make _PyDict_LoadGlobal thread safe by DinoV · Pull Request #117529

gh-112075: Make _PyDict_LoadGlobal thread safe by DinoV · Pull Request #117529 · python/cpython

Conversation

Currently _PyDict_LoadGlobal is using the non-thread safe _Py_dict_lookup and isn't locking the dictionaries. This switches to using the thread safe version and modifies the function to return a new reference.

It also adds an assertion for _Py_dict_lookup that the dictionary should be locked, and fixes up ordered dict to use the thread safe version where the assertion trips.

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

DinoV marked this pull request as ready for review

April 4, 2024 01:03

DinoV changed the title ~~Draft gh-112075: Make _PyDict_LoadGlobal thread safe~~ gh-112075: Make _PyDict_LoadGlobal thread safe

Apr 4, 2024

🤖 New build scheduled with the buildbot fleet by @colesbury for commit 548a7c2 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

		* key hash failed, key comparison failed, ...). Return NULL if the key doesn't
		* exist. Return the value if the key exists.
		*
		* Returns a new reference.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we will want it to return a possibly deferred reference soon, but this is good for now.

		#ifdef Py_GIL_DISABLED
		/* namespace 1: globals */
		ix = _Py_dict_lookup(globals, key, hash, &value);
		ix = _Py_dict_lookup_threadsafe(globals, key, hash, &value);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this pattern might be simpler if we define _Py_dict_lookup_threadsafe in the default build as _Py_dict_lookup_threadsafe + Py_XNewRef().

We could use it in dict_subscript and dict_get_impl as well.

diegorusso pushed a commit to diegorusso/cpython that referenced this pull request

Apr 17, 2024

Make _PyDict_LoadGlobal threadsafe

DinoV deleted the nogil_dict_loadglobal branch

May 31, 2024 18:22

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels