-
-
Notifications
You must be signed in to change notification settings - Fork 30.7k
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
gh-115999: Specialize LOAD_ATTR
for instance and class receivers in free-threaded builds
#128164
Open
mpage
wants to merge
47
commits into
python:main
Choose a base branch
from
mpage:gh-115999-load-attr-instance-merged
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+508
−173
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Look up a unicode key in an all unicode keys object along with the keys version, assigning one if not present. We need a keys version that is consistent with presence of the key for use in the guards.
Reading the shared keys version and looking up the key need to be performed atomically. Otherwise, a key inserted after the lookup could race with reading the version, causing us to incorrectly specialize that no key shadows the descriptor.
Everything starts out disabled
* Check that the type hasn't changed across lookups * Descr is now an owned ref
…_INST_ATTR_FROM_DICT
- Use atomic load for value - Use _Py_TryIncrefCompareStackRef for incref
- Use atomics and _Py_TryIncrefCompareStackRef in _LOAD_ATTR_SLOT - Pass type version during specialization
- Check that fget is deferred - Pass tp_version
Macros should be treated as terminators when searching for the assignment target of an expression involving PyStackRef_FromPyObjectNew
All instance loads are complete!
- Use atomics where appropriate - Only cache deferred values
If specialization fails, `X.mykey` will be performed again using the new mro set by __eq__. Check that the process doesn't crash, since that's what we care about.
Always use the type version from the __getattribute__ lookup (even if its zero). The guard will fail if the type changes afterwards.
This matches the previous implementation and causes failures to specialize due to the presence of both __getattr__ and __getattribute__ to be classified correctly (rather than being classified as being out of type versions).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR finishes specialization for
LOAD_ATTR
in the free-threaded build by adding support for class and instance receivers.The bulk of it is dedicated to making the specialized instructions and the specialization logic thread-safe. This consists of using atomics / locks in the appropriate places, avoiding races in specialization related to reloading versions, and ensuring that the objects stored in inline caches remain valid when accessed (by only storing deferred objects). See the section on "Thread Safety" below for more details.
Additionally, making this work required a few unrelated changes to fix existing bugs or work around differences between the two builds that results from only storing deferred values (which causes in specialization failures in the free-threaded build when a value that would be stored in the cache is not deferred):
PyStackRef_FromPyObjectNew
.test_descr.MiscTests.test_type_lookup_mro_reference
to work when specialization fails (and also be a behavorial test).test_capi.test_type.TypeTests.test_freeze_meta
when running refleaks tests on free-threaded builds. Specialization failure triggers an existing bug.Single-threaded Performance
We're leaving a bit of perf on the table by only storing deferred objects: we can't specialize attribute lookups that resolve to class attributes (e.g. counters, settings). I haven't measured how much perf we're giving up, but I'd like to address that in a separate PR.
Scalability
The
object_cfunction
andpymethod
benchmarks are improved (1.4x slower -> 14.3x faster, 1.8x slower -> 13.0x faster, respectively). Other benchmarks appear unchanged.I would expect that
cmodule_function
would also improve, but it looks like the benchmark is bottlenecked on increfing theint.__floor__
method that is returned from the call to_PyObject_LookupSpecial
inmath_floor
(the incref happens in_PyType_LookupRef
, which is called byPyObject_LookupSpecial
):cpython/Modules/mathmodule.c
Line 1178 in 3879ca0
Raw numbers:
Thread Safety
Thread safety of specialized instructions is addressed in a few different ways:
_LOAD_ATTR_WITH_HINT
.Thread safety of specialization is addressed using similar techniques:
Stats
Following the instructions in the comment preceding
specialize_attr_loadclassattr
, I collected stats for both this PR and its merge base using./python -m test_typing test_re test_dis test_zlib
and compared them usingTools/scripts/summarize_stats.py
. The results forLOAD_ATTR
are nearly identical and are consistent with results from comparing the merge base against itself:--disable-gil
builds #115999