gh-115999: Specialize loading attributes from modules in free-threaded builds #127711
+437
−179
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
mpage
added
the
🔨 test-with-refleak-buildbots
bedevere-bot
removed
the
🔨 test-with-refleak-buildbots
|
Refleaks build failures for the default build are preexisting. The free-threaded failure looks like its caused by this PR. |
…y_decrefs_once Refactor the test so that the specialized and unspecialized implementation of loading the argument to sys.getrefcount use the same refcounting approach. Previously, the argument would be evaluated by loading an attribute from a module. This specializes to LOAD_ATTR_MODULE. In free-threaded builds the unspecialized form of LOAD_ATTR always creates a new reference for its result, while the specialized form does not create a reference if the result uses deferred refcounting. This causes a difference in the result returned from sys.getrefcount, depending on whether or not the bytecode has been specialized (e.g. on runs > 1 in refleak tests). The refactored version uses LOAD_GLOBAL, whose specialized and unspecialized forms both do not create references when the result uses deferred refcounting. Also refactor the test to handle the difference in the result returned from sys.getrefcount in default builds (includes the temporary reference on the operand stack) and free-threaded builds (no temporary reference is created for deferred values).
|
!buildbot nogil refleak |
|
🤖 New build scheduled with the buildbot fleet by @mpage for commit 550f955 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
colesbury
approved these changes
Dec 10, 2024
| @@ -665,19 +667,46 @@ def test_c_subclass_of_heap_ctype_with_del_modifying_dunder_class_only_decrefs_o | |||
| del subclass_instance | |||
|
|
|||
| # Test that setting __class__ modified the reference counts of the types | |||
| # | |||
There was a problem hiding this comment.
A bit of an aside, but I'm skeptical of the value of this test considering the increasing complexity. If we want to make sure we don't decref too many times, we should test that we don't crash. Depending on the exact refcounts feels too likely to break with implementation changes.
There was a problem hiding this comment.
Agreed. Checking that we don't crash sounds like a better approach to me. Filed #127881 to track that.
mpage
added
the
🔨 test-with-buildbots
bedevere-bot
removed
the
🔨 test-with-buildbots
|
!buildbot AMD64 Android |
|
Buildbot failures look unrelated to this PR:
|
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 is the first in a series of PRs to specialize the
LOAD_ATTRfamily of opcodes in free-threaded builds. PRs specializingLOAD_ATTRfor instance and class receivers will follow.We use the same approach that was used for specialization of
LOAD_GLOBALin free-threaded builds:_CHECK_ATTR_MODULEis renamed to_CHECK_ATTR_MODULE_PUSH_KEYS; it pushes the keys object for the following_LOAD_ATTR_MODULE_FROM_KEYS(nee_LOAD_ATTR_MODULE). This arrangement avoids having to recheck the keys version._LOAD_ATTR_MODULEis renamed to_LOAD_ATTR_MODULE_FROM_KEYS; it loads the value from the keys object pushed by the preceding_CHECK_ATTR_MODULE_PUSH_KEYSat the cached index.A few other changes were necessary to support this arrangement:
POP_DEAD_INPUTS()to the cases generator. It removes dead inputs from the stack and updates the stack pointer. This is used in_LOAD_ATTR_MODULE_FROM_KEYSto remove the keys object from the stack before deopting._LOAD_ATTR_MODULE. This is used by the tier2 optimizer when we successfully erase a_CHECK_ATTR_MODULE_PUSH_KEYSbut cannot convert the conjugate_LOAD_ATTR_MODULE_FROM_KEYSinto aLOAD_CONST.specializeandunspecializehelpers. This isn't strictly necessary, but it makes the implementation of gradually adding specialization support cleaner.test_c_subclass_of_heap_ctype_with_del_modifying_dunder_class_only_decrefs_onceso that it works with deferred refcounting in the free-threaded build. Also refactored the test so that the evaluation of the argument expression to sys.getrefcount always uses deferred refcounting regardless of whether or not it's specialized.Thread Safety
Specialization
A critical section on the module's dictionary is held during specialization. This prevents concurrent mutation of the dictionary and its keys version.
_PyDict_GetKeysVersionForCurrentStateis used to assign the keys version, ensuring that the keys object is freed using QSBR.UOPS
md_dictfield, but not the dictionary itself, is immutable.Single-threaded Performance
Scaling Benchmarks
This improves the scaling of calls to module functions (2.1x slower -> 13.9x faster), while other benchmarks appear unchanged.
--disable-gilbuilds #115999