gh-115999: Specialize `STORE_ATTR` in free-threaded builds.#127838

nascheme · 2024-12-11T18:35:07Z

Fix thread safety issues with specialized opcodes (STORE_ATTR_INSTANCE_VALUE, STORE_ATTR_SLOT, STORE_ATTR_WITH_HINT). Need a combination of locks and atomics to be safe.
Fix thread safety issues with _Py_Specialize_StoreAttr . Avoid using borrowed references. Save and store the tp_version_tag from the beginning of the specialization process since it might change. Use helper functions to update opcode.
Add unit tests to ensure specialization is happening in free-threaded builds. Add addtional tests to try to trigger data races.

Issue: Make the specializing interpreter thread-safe in --disable-gil builds #115999

Python/bytecodes.c

mpage

Thanks for taking this on! This looks like a regression on the default build. I haven't had a chance to dig into it, but I suspect it might either be due to the check that Sam flagged in _STORE_ATTR_INSTANCE_VALUE or the change to when we read the type version in _Py_Specialize_StoreAttr. It looks like the richards benchmark is the most heavily affected, so that might be a good isolated benchmark to use for debugging.

Lib/test/test_opcache.py

Python/specialize.c

nascheme · 2024-12-13T05:35:42Z

Thanks for taking this on! This looks like a regression on the default build. I haven't had a chance to dig into it, but I suspect it might either be due to the check that Sam flagged in _STORE_ATTR_INSTANCE_VALUE or the change to when we read the type version in _Py_Specialize_StoreAttr. It looks like the richards benchmark is the most heavily affected, so that might be a good isolated benchmark to use for debugging.

Based on my benchmarking my second commit, the regression with richards is gone. Likely it was the fact that STORE_ATTR_INSTANCE_VALUE was not actually working.

Regarding the tp_version_tag not getting set due to type_get_version() being hoisted, fixing it as you suggest by using _PyType_LookupRefAndVersion is a bit complex to do. So, I deferred doing that for now. I'll look at it again.

nascheme · 2024-12-13T06:14:29Z

Regarding the tp_version_tag not getting set due to type_get_version() being hoisted, fixing it as you suggest by using _PyType_LookupRefAndVersion is a bit complex to do. So, I deferred doing that for now. I'll look at it again.

This was actually not hard to fix. I was thinking that _PyType_LookupRefAndVersion would not set the tag in the case the lookup fails. However, that's not the case.

Python/bytecodes.c

nascheme · 2024-12-13T19:33:30Z

I rebased on main to resolve merge conflicts and force pushed.

mpage

I left a couple of small comments inline, but LGTM overall.

Python/specialize.c

* Fix locking for `STORE_ATTR_INSTANCE_VALUE`. Create `_GUARD_TYPE_VERSION_AND_LOCK` so that object stays locked and `tp_version_tag` cannot change. Fix inverted logic bug that caused erroneous deopt. * Fix locking for `_STORE_ATTR_WITH_HINT`. Double check that `_PyObject_GetManagedDict()` hasn't disappeared since we locked the dict. - Pass `tp_version_tag` to `specialize_dict_access()`, ensuring the version we store on the cache is the correct one (in case of it changing during the specalize analysis). - Split `analyze_descriptor` into `analyze_descriptor_load` and `analyze_descriptor_store` since those don't share much logic. Add `descriptor_is_class` helper function. - In `specialize_dict_access`, double check `_PyObject_GetManagedDict()` in case we race and dict was materialized before the lock.

If the type is new and a version tag hasn't yet been assigned, we would fail to specialize it. Use `_PyType_LookupRefAndVersion()` instead of `type_get_version()`, which will assign a version.

Use provided value of `tp_version` to store in cache.

This also fixes the case if the dict is replaced with a different one.

The function is only used in with-GIL builds.

For `specialize_dict_access_inline()`, we need to lock the keys object. * Add `_PyDictKeys_StringLookupSplit` which does required locking and use in place of `_PyDictKeys_StringLookup`. * Change `_PyObject_TryGetInstanceAttribute` to use that function in the case of split keys. * Add `unicodekeys_lookup_split` helper which allows code sharing between `_Py_dict_lookup` and `_PyDictKeys_StringLookupSplit`.

mpage

Nice! Just a couple of small comments inline.

Objects/dictobject.c

mpage

LGTM!

Benchmark results:

~2% faster on free-threaded builds.
~neutral on default builds.

…thongh-127838) * Add `_PyDictKeys_StringLookupSplit` which does locking on dict keys and use in place of `_PyDictKeys_StringLookup`. * Change `_PyObject_TryGetInstanceAttribute` to use that function in the case of split keys. * Add `unicodekeys_lookup_split` helper which allows code sharing between `_Py_dict_lookup` and `_PyDictKeys_StringLookupSplit`. * Fix locking for `STORE_ATTR_INSTANCE_VALUE`. Create `_GUARD_TYPE_VERSION_AND_LOCK` uop so that object stays locked and `tp_version_tag` cannot change. * Pass `tp_version_tag` to `specialize_dict_access()`, ensuring the version we store on the cache is the correct one (in case of it changing during the specalize analysis). * Split `analyze_descriptor` into `analyze_descriptor_load` and `analyze_descriptor_store` since those don't share much logic. Add `descriptor_is_class` helper function. * In `specialize_dict_access`, double check `_PyObject_GetManagedDict()` in case we race and dict was materialized before the lock. * Avoid borrowed references in `_Py_Specialize_StoreAttr()`. * Use `specialize()` and `unspecialize()` helpers. * Add unit tests to ensure specializing happens as expected in FT builds. * Add unit tests to attempt to trigger data races (useful for running under TSAN). * Add `has_split_table` function to `_testinternalcapi`.

nascheme added the topic-free-threading label Dec 11, 2024

bedevere-appbot mentioned this pull request Dec 11, 2024
Make the specializing interpreter thread-safe in --disable-gil builds #115999
Closed

nascheme changed the title ~~gh-115999: Enable specialization of STORE_ATTR free-threaded builds.~~gh-115999: Specialize STORE_ATTR in free-threaded builds.Dec 11, 2024

nascheme added the skip news label Dec 11, 2024

nascheme marked this pull request as ready for review December 11, 2024 21:54

nascheme requested a review from markshannon as a code owner December 11, 2024 21:54

bedevere-appbot added the awaiting core review label Dec 11, 2024

nascheme requested a review from mpage December 11, 2024 21:54

colesbury reviewed Dec 12, 2024
View reviewed changes

Python/bytecodes.cShow resolvedHide resolved
Python/bytecodes.c Outdated Show resolvedHide resolved

mpage reviewed Dec 12, 2024
View reviewed changes

Lib/test/test_opcache.py Outdated Show resolvedHide resolved
Python/specialize.c Outdated Show resolvedHide resolved
Python/specialize.c Outdated Show resolvedHide resolved
Python/specialize.c Outdated Show resolvedHide resolved
Python/specialize.c Outdated Show resolvedHide resolved

colesbury reviewed Dec 13, 2024
View reviewed changes

Python/bytecodes.c Outdated Show resolvedHide resolved

colesbury reviewed Dec 13, 2024
View reviewed changes

Python/bytecodes.c Outdated Show resolvedHide resolved

nascheme force-pushed the gh-115999-specialize-store-attr branch from f920dcd to 4c484abCompare December 13, 2024 19:17

mpage reviewed Dec 16, 2024
View reviewed changes

Python/specialize.c Outdated Show resolvedHide resolved
Python/specialize.c Outdated Show resolvedHide resolved

nascheme requested a review from methane as a code owner December 17, 2024 21:59

nascheme added 9 commits December 17, 2024 22:15

Enable specialization of STORE_ATTR free-threaded builds.
083cbe9

Small optimization for STORE_ATTR specialize.
6fb6e4a
If the type is new and a version tag hasn't yet been assigned, we would fail to specialize it. Use `_PyType_LookupRefAndVersion()` instead of `type_get_version()`, which will assign a version.

Fix race in specialize_dict_access_inline().
75d9a53
Use provided value of `tp_version` to store in cache.

Use correct type of load for tp_version_tag.
476058d

Avoid overwritting 'dict', needed for unlock.
756f939
This also fixes the case if the dict is replaced with a different one.

Add additional tests for STORE_ATTR.
104d972

Remove unneeded atomic load.
4981652
The function is only used in with-GIL builds.

nascheme force-pushed the gh-115999-specialize-store-attr branch from 699f4e9 to 6bf9016Compare December 18, 2024 06:16

mpage reviewed Dec 18, 2024
View reviewed changes

Objects/dictobject.c Outdated Show resolvedHide resolved
Objects/dictobject.c Outdated Show resolvedHide resolved

Code cleanup, remove unneeded branches.
9015a3f

mpage approved these changes Dec 19, 2024
View reviewed changes

bedevere-appbot added awaiting merge and removed awaiting core review labels Dec 19, 2024

nascheme added 2 commits December 18, 2024 20:38

Merge branch 'main' intopythongh-115999-specialize-store-attr
14ae6b4

Merge branch 'main' intopythongh-115999-specialize-store-attr
06a7baf

nascheme merged commit 1b15c89 into python:mainDec 19, 2024
55 checks passed

bedevere-appbot removed the awaiting merge label Dec 19, 2024

Uh oh!

gh-115999: Specialize STORE_ATTR in free-threaded builds.#127838

gh-115999: Specialize STORE_ATTR in free-threaded builds. #127838

Uh oh!

Conversation

nascheme commented Dec 11, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mpage left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nascheme commented Dec 13, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nascheme commented Dec 13, 2024

Uh oh!

Uh oh!

Uh oh!

nascheme commented Dec 13, 2024

Uh oh!

mpage left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mpage left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mpage left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gh-115999: Specialize `STORE_ATTR` in free-threaded builds.#127838

gh-115999: Specialize `STORE_ATTR` in free-threaded builds. #127838

nascheme commented Dec 11, 2024•
edited
Loading

nascheme commented Dec 13, 2024•
edited
Loading