gh-122029: Do not unpack method for legacy tracing anymore#130898
Merged
gaogaotiantian merged 3 commits intopython:mainfrom Mar 11, 2025
Merged
gh-122029: Do not unpack method for legacy tracing anymore#130898gaogaotiantian merged 3 commits intopython:mainfrom
gaogaotiantian merged 3 commits intopython:mainfrom
Conversation
Member
|
I took another look at It might be worth it if it were to unlock other optimizations, but let's go with this much simpler fix for now. |
markshannon
reviewed
Mar 10, 2025
markshannon
approved these changes
Mar 10, 2025
Member
markshannon
left a comment
There was a problem hiding this comment.
I've suggested one slight tweak to the test, otherwise looks good.
Co-authored-by: Mark Shannon <mark@hotpy.org>
This file contains hidden or 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
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.
INSTRUMENTED_CALLandINSTRUMENTED_CALL_KWnow unpack the method before monitoring, so they are not affected by this change. The bytecode that this affects isINSTRUMENTED_CALL_FUNCTION_EX.For
INSTRUMENTED_CALL_FUNCTION_EX, we do not unpack the callable, instead, we usePyObject_Calldirectly on the callable. Because of this,sys.monitoringdoes not know this eventually calls into a C function, because the callable is a Python method, soc_returnevent will not be generated.With the current code, because we unpack the method in
sys.setprofile, we will generate an unmatchedc_call, which is horrible.The ideal result is probably have a consistent result for all three bytecodes, but that requires some refactoring for
CALL_FUNCTION_EXwhich @markshannon was kind of against. If we can't achieve that, we should at least generated paired events - so we don't have ac_callwithout itsc_return. That's why I removed the unpack code for legacy tracing.