Skip to content

gh-122029: Do not unpack method for legacy tracing anymore #130898

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

Merged
merged 3 commits into from
Mar 11, 2025

Conversation

gaogaotiantian
Copy link
Member

@gaogaotiantian gaogaotiantian commented Mar 5, 2025

INSTRUMENTED_CALL and INSTRUMENTED_CALL_KW now unpack the method before monitoring, so they are not affected by this change. The bytecode that this affects is INSTRUMENTED_CALL_FUNCTION_EX.

For INSTRUMENTED_CALL_FUNCTION_EX, we do not unpack the callable, instead, we use PyObject_Call directly on the callable. Because of this, sys.monitoring does not know this eventually calls into a C function, because the callable is a Python method, so c_return event will not be generated.

With the current code, because we unpack the method in sys.setprofile, we will generate an unmatched c_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_EX which @markshannon was kind of against. If we can't achieve that, we should at least generated paired events - so we don't have a c_call without its c_return. That's why I removed the unpack code for legacy tracing.

@markshannon
Copy link
Member

I took another look at CALL_FUNCTION_EX and the refactor to unpack the bound method would require a fair amount of extra logic to handle the self argument, either unpacking self and the remaining args into a new array, or prepending self to the args tuple.

It might be worth it if it were to unlock other optimizations, but let's go with this much simpler fix for now.

Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've suggested one slight tweak to the test, otherwise looks good.

@gaogaotiantian gaogaotiantian merged commit 8b1edae into python:main Mar 11, 2025
42 checks passed
@gaogaotiantian gaogaotiantian deleted the fix-call-function-ex branch March 11, 2025 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants