GH-128375: Better instrument for FOR_ITER#128445
Conversation
markshannon
requested review from
ericsnowcurrently and
iritkatriel
as code owners
Lib/test/test_code.py
Outdated
| code_offset_to_line(code, src)-base, | ||
| code_offset_to_line(code, left)-base, | ||
| code_offset_to_line(code, right)-base |
There was a problem hiding this comment.
| code_offset_to_line(code, src)-base, | |
| code_offset_to_line(code, left)-base, | |
| code_offset_to_line(code, right)-base | |
| code_offset_to_line(code, src) - base, | |
| code_offset_to_line(code, left) - base, | |
| code_offset_to_line(code, right) - base, |
Python/bytecodes.c
Outdated
| tier1 inst(INSTRUMENTED_END_FOR, (receiver, value -- receiver)) { | ||
| /* Need to create a fake StopIteration error here, | ||
| * to conform to PEP 380 */ | ||
| // * to conform to PEP 380 ED_NO*/ |
Python/bytecodes.c
Outdated
| * This has the benign side effect that if value is | ||
| * finalized it will see the location as the FOR_ITER's. | ||
| */ | ||
| frame->instr_ptr = prev_instr; |
There was a problem hiding this comment.
Is there a precedent for this? Seems like the kind of thing that can cause confusion/problems later on.
There was a problem hiding this comment.
The tier 2 code generator doesn't like it either. I think I'll need to add a new annotation "no_saved_ip" to tell the code generator not to update frame->instr_ptr.
Any suggestions for a better name than "no_saved_ip"?
|
I was wondering if we couldn't, instead of putting in |
|
Trying this PR compared to fa985be, more of my tests fail (on a work-in-progress branch to account for BRANCH_LEFT etc). I'll debug my code, but I wish there were a way to get a concrete plan to work from. |
I think that would be worse overall, for a few reasons:
|
…ave the IP to frame->instr_ptr
|
Is this intended? continue.py: for i in range(10):
a = i
continue # 3
a = 99
assert a == 9 # 5Disassembled with this branch: sys.monitoring events using run_sysmon.py: Is it right that the starred line has a BRANCH_RIGHT event from a POP_ITER instruction? |
No. It should be from the |
|
Two more examples of branch event surprises. run_sysmon.py is used to see the sys.monitoring events. These are using commit cbe6c7c from PR #128445. ifdebug.pyfor value in [True, False]:
if value:
if __debug__:
x = 4
else:
x = 6Disassembled: Events: The starred line is a BRANCH event from a NOT_TAKEN instruction. 1176_async.pyimport asyncio
async def async_gen():
yield 4
async def async_test():
async for i in async_gen():
print(i + 8)
asyncio.run(async_test())Disassembled: Events: Here there are no BRANCH events at all. I would expect some for the |
|
These examples are really helpful, but I don't think they are relevant to this PR. The |
| * the FOR_ITER as the previous instruction. | ||
| * This has the benign side effect that if value is | ||
| * finalized it will see the location as the FOR_ITER's. | ||
| */ |
There was a problem hiding this comment.
Can't we just teach POP_ITER to look at prev_instr-1?
There was a problem hiding this comment.
prev_instr is the previously executed instruction, which is probably not the previous instruction in the code.
There is no way for POP_ITER to tell where the jump came from if we overwrite frame->instr_ptr
FOR_ITER_LIST specialization.
|
The Is this intentional or do you want me to raise an issue about this? Or has an issue perhaps been raised already? |
Summary: See [GH-128445](python/cpython#128445). Reviewed By: alexmalyshev Differential Revision: D81142355 fbshipit-source-id: 33c7eb51bd64722229df88691619a3e9fcbede6b
4 participants
This PR:
POP_TOPand the end of aforloop toPOP_ITER(it pops the iterator)NOT_TAKENfollowingFOR_ITERand instruments theFOR_ITERandPOP_ITERinsteadEXTENDED_ARGSinco_branches().The last one isn't strictly related to the issue, but came up while fixing the main issue.
Changing the instrumentation pattern has two effects:
NOT_TAKEN) per iteration, at the cost of one additional dispatch when the loop is exited.DISABLEd, theFOR_ITERcan be specialized and jitted.Performance is neutral on the standard benchmarks for tier1, as expected.
FOR_ITERis poor asDISABLEdoesn't remove the instrumentation. #128375