-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
gh-107265: Remove all ENTER_EXECUTOR when execute _Py_Instrument #108539
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
Conversation
corona10
commented
Aug 27, 2023
•
edited by bedevere-bot
Loading
edited by bedevere-bot
- Issue: Decoding instructions should handle ENTER_EXECUTOR #107265
Python/instrumentation.c
Outdated
@@ -1577,6 +1551,20 @@ _Py_Instrument(PyCodeObject *code, PyInterpreterState *interp) | |||
return 0; | |||
} | |||
int code_len = (int)Py_SIZE(code); | |||
if (code->co_executors != NULL && code->co_executors->size > 0 ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address by @markshannon's comment
: #108482 (comment)
Python/instrumentation.c
Outdated
bool instrumented = is_instrumented(opcode); | ||
if (instrumented) { | ||
opcode = DE_INSTRUMENT[opcode]; | ||
assert(opcode != 0); | ||
} | ||
assert(opcode != ENTER_EXECUTOR); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was added to test #107265 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the test could reproduce what you did in that comment?
gentle ping? @gvanrossum @markshannon |
Sorry! I started a review but got distracted. Give me 1-2 days. |
The code changes look fine, but there is no test. Is it possible to add a test for this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a refactoring suggestion. And I agree with Mark that a test would be nice.
Thanks Guido and Mark, I will update the PR soon :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Though I would put spaces on both sides of +=.
@@ -1698,3 +1698,31 @@ def run(): | |||
self.assertEqual(caught, "inner") | |||
finally: | |||
sys.monitoring.set_events(TEST_TOOL, 0) | |||
|
|||
|
|||
class TestOptimizer(MonitoringTestBase, unittest.TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a test, but not sure what you wanted :)
Without this PR, Python/instrumentation.c#L1589 will not be able to pass the assert through this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this test supposed to do? When I copy this test into the main branch, it passes (in debug mode, so failing asserts would cause crashes), so I don't think this proves your fix works.
By adding some dis.dis(test_func, adaptive=True)
calls to the test I think I see the difference -- in main, an ENTER_EXECUTOR opcode remains in the code object, whereas with this PR, the ENTER_EXECUTOR opcode appears after the function is called, but disappears again after the second set_local_events call.
You could check for that ENTER_EXECUTOR (there are examples in test_capi/test_misc.py) but it would be nice if you had example code that actually crashed or triggered an assert (in debug mode) in main without your fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm,, interesting, It looks like I watched the Hallucination when I wrote the test in the first place :(
I will soon update the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See: #108539 (comment)
super(TestOptimizer, self).tearDown() | ||
_testinternalcapi.set_optimizer(self.old_opt) | ||
|
||
def test_for_loop(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like @markshannon to review this part at least.
Gentle ping :) |
Guido, Reproduce in main branch
Run In main branch
Run with PR
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I trust the output you posted, and I understand what's happening there.
Now I just have one question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Now it LGTM and you don't have to wait for Mark.
|
Unrelated! |