Uh oh!
There was an error while loading. Please reload this page.
gh-107265: Remove all ENTER_EXECUTOR when execute _Py_Instrument - #108539
Conversation
| 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.
Address by @markshannon's comment
: #108482 (comment)
| opcode = DE_INSTRUMENT[opcode]; | ||
| assert(opcode != 0); | ||
| } | ||
| assert(opcode != ENTER_EXECUTOR); |
There was a problem hiding this comment.
Maybe the test could reproduce what you did in that comment?
Uh oh!
There was an error while loading. Please reload this page.
corona10
commented
Aug 31, 2023
gentle ping? @gvanrossum@markshannon |
gvanrossum
commented
Aug 31, 2023
Sorry! I started a review but got distracted. Give me 1-2 days. |
markshannon
commented
Aug 31, 2023
The code changes look fine, but there is no test. Is it possible to add a test for this? |
gvanrossum
left a comment
There was a problem hiding this comment.
I have a refactoring suggestion. And I agree with Mark that a test would be nice.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
corona10
commented
Sep 1, 2023
Thanks Guido and Mark, I will update the PR soon :) |
gvanrossum
left a comment
There was a problem hiding this comment.
LGTM. Though I would put spaces on both sides of +=.
| sys.monitoring.set_events(TEST_TOOL, 0) | ||
| class TestOptimizer(MonitoringTestBase, unittest.TestCase): |
There was a problem hiding this comment.
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.
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.
Hmm,, interesting, It looks like I watched the Hallucination when I wrote the test in the first place :(
I will soon update the PR.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| super(TestOptimizer, self).tearDown() | ||
| _testinternalcapi.set_optimizer(self.old_opt) | ||
| def test_for_loop(self): |
corona10
commented
Sep 6, 2023
Gentle ping :) |
Guido, Reproduce in main branchRun In main branchRun with PR |
gvanrossum
left a comment
There was a problem hiding this comment.
I trust the output you posted, and I understand what's happening there.
Now I just have one question.
Uh oh!
There was an error while loading. Please reload this page.
gvanrossum
left a comment
There was a problem hiding this comment.
Great! Now it LGTM and you don't have to wait for Mark.
bedevere-bot
commented
Sep 7, 2023
|
corona10
commented
Sep 7, 2023
Unrelated! |
Uh oh!
There was an error while loading. Please reload this page.