gh-130373: Avoid locking in _LOAD_ATTR_WITH_HINT #130372
Conversation
DinoV
changed the title
Python/bytecodes.c
Outdated
| POP_INPUT(dict); | ||
| DEOPT_IF(true); | ||
| } | ||
| PyDictUnicodeEntry *ep = DK_UNICODE_ENTRIES(dict->ma_keys) + hint; | ||
| PyDictUnicodeEntry *ep = DK_UNICODE_ENTRIES(dk) + hint; | ||
| if (ep->me_key != name) { |
There was a problem hiding this comment.
I think the read of ep->me_key needs to be atomic (can be relaxed).
Python/bytecodes.c
Outdated
| } else if (dk != FT_ATOMIC_LOAD_PTR(dict->ma_keys)) { | ||
| PyStackRef_CLOSE(attr); | ||
| POP_INPUT(dict); | ||
| DEOPT_IF(true); | ||
| } |
There was a problem hiding this comment.
I don't think we need this check dk != FT_ATOMIC_LOAD_PTR(dict->ma_keys). Even if the dict->ma_keys changed, we should still have loaded a valid previous value at this point.
There was a problem hiding this comment.
That seems right to me, I think that means we also don't need the check here? https://github.com/python/cpython/blob/main/Objects/dictobject.c#L1527
There was a problem hiding this comment.
Yeah. There's also a similar check a few lines below that in the same function:
Lines 1547 to 1550 in 69426fc
| @@ -2286,38 +2286,40 @@ dummy_func( | |||
| } | |||
|
|
|||
| op(_LOAD_ATTR_WITH_HINT, (hint/1, owner, dict: PyDictObject * -- attr)) { | |||
| PyDictKeysObject *dk = FT_ATOMIC_LOAD_PTR(dict->ma_keys); | |||
There was a problem hiding this comment.
I think that we need to check the equivalent of ensure_shared_on_read either here or in _CHECK_ATTR_WITH_HINT.
DinoV
force-pushed
the
nolock_loadattr_with_hint
branch
3 times, most recently
from
163dd7b to
6f8b1d7
Compare
DinoV
force-pushed
the
nolock_loadattr_with_hint
branch
from
6f8b1d7 to
15c28cb
Compare
Adds a lock-free lookup of the hinted location
https://github.com/facebookexperimental/free-threading-benchmarking/tree/main/results/bm-20250219-3.14.0a5+-6a1fe7e-NOGIL