Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Python/instrumentation.c Outdated
| return0; | ||
| } | ||
| intcode_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
| 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?
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.
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.
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.
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 +=.
| sys.monitoring.set_events(TEST_TOOL, 0) | ||
| classTestOptimizer(MonitoringTestBase, unittest.TestCase): |
corona10Sep 1, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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)
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) | ||
| deftest_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.
corona10 commented Sep 6, 2023
Gentle ping :) |
corona10 commented Sep 6, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Guido, Reproduce in main branchRun In main branchRun with PR |
gvanrossum left a 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.
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.
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.
bedevere-bot commented Sep 7, 2023
|
corona10 commented Sep 7, 2023
Unrelated! |
Uh oh!
There was an error while loading. Please reload this page.