gh-112075: Make instance attributes stored in inline "dict" thread safe

[DinoV]

Adds some locking around when we de-materialize a dictionary.

Also adds a comment where we'll need qsbr support for materialization.

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

[markshannon]

This looks inefficient, we really don't want locks in the fast path for attribute lookup.

Rather than putting locks around things, maybe we can revisit the way we handle objects with managed dictionaries to consider free-threading?

[DinoV]

This looks inefficient, we really don't want locks in the fast path for attribute lookup.

Rather than putting locks around things, maybe we can revisit the way we handle objects with managed dictionaries to consider free-threading?

I assume you're referring to the latest unfinished version (as previous one only added a lock for dematerialization which as @colesbury pointed out isn't correct)? If so there aren't actually any locking for lookups in the fast path... There is an read w/ acquire semantics, but on strongly-ordered systems like x64 that's no different than a normal read anyway. It does introduce an incref/decref when reading from the dictionary. Also FWIW I think I don't need the spinning in _PyDictOrValues_TryGetDict and _PyDictOrValues_TryGetValues or the usage of _PYDICTORVALUES_UPDATING as I have in the current version.

I think we could get rid of the incref though if we disabled dematerialization in free-threading, but there's still going to be some ref count checks to see if the object is local, and if not to mark it as shared so that we'll free the values via QSBR. I'm not sure that can be avoided.

[DinoV]

I've run into an existing complication with the existing solution I've been trying. The limitation in the byte code generator " Until the last DEOPT_IF, no objects may be allocated, INCREFed, or DECREFed." makes this leak the dict on de-opt. We could of course follow the error pattern of if (cond) { Py_DECREF(dict); DEOPT_IF(true); } but maybe that's a non-starter...

It also occurred to me that we're not just racing with de-materialization of the dict, but we're also racing with someone assigning to __dict__ and freeing the existing dict.

So an alternate plan might be:

1. Don't incref the dicts for read
2. Don't de-materialize shared dicts (but still de-materialize thread-local dicts)
3. On assignment to __dict__ if the existing dict is shared then scheduled it to be freed via QSBR, otherwise free it immediately.

[markshannon]

faster-cpython/ideas#651

[bedevere-bot]

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

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

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @DinoV for commit 4b85255 🤖

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

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @DinoV for commit fac83b7 🤖

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

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @DinoV for commit fac83b7 🤖

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

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @DinoV for commit ac503e8 🤖

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