-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
GH-108866: Change optimizer API and contract #109144
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
GH-108866: Change optimizer API and contract #109144
Conversation
…execute at least one instruction.
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.
The more I think about this, the more I think that the problem with signature of the executor is that it has too many return values:
- An error indicator
- The frame where the error (or exit) occurred
- The instruction pointer where it occurred
- The stack pointer at that time
Both the frame and the instruction pointer are needed to continue execution and also for error handling, and both can differ from the value before the executor runs. Same for the stack pointer (though we always sync that through the frame object).
Things would be neater if these were all explicit and we didn't have to recover one or the other through tstate->current_frame
.
The optimizer inherits this awkwardness because it promises to also run the executor. (And why is that? It seems to make things more complicated, and it means the opcode isn't replaced until after that first run -- won't that confuse other threads?)
The requirement to include the JUMP_BACKWARD
instruction in the input to the optimizer causes a bunch of extra work which I'm also not keen on. And, despite what I said in in gh-108866, I actually still don't completely understand why we need this.
In any case maybe we need to split the two changes.
Python/bytecodes.c
Outdated
while(oparg > 255) { | ||
oparg >>= 8; | ||
src--; | ||
} |
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.
Clever. I'd add assert(src->op.code == EXTENDED_ARG)
.
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 don't think this assert is correct, since it could be an INSTRUMENTED_LINE
or something. Ditto for the matching assert in _PyOptimizer_BackEdge
.
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.
Ouch. Can instrumentation really overwrite EXTENDED_ARG
? @markshannon
The frame pointer, instruction pointer and stack pointer are all part of the VM state. The only reason we passed these values as arguments or return value was for efficiency. It reduces the number of memory accesses at the point of transfer. If it turns out that most callees don't need them, we can just remove the parameter from the API. The difference with this PR is that the return value is meaningful, not just a cached value. The
|
Thanks for the changes. I believe we decided offline to hold off on this until Brandt has had an opportunity to change |
This is now obsolete |
Partial implementation of #108866
_PyInterpreterFrame *
to_Py_CODEUNIT *
, returning the next instruction to executeSince the uop and optimizer and executor start at the top of the loop, starting at the
JUMP_BACKWARD
means that they always execute one or more instructions.ENTER_EXECUTOR
is more efficient as it doesn't need to worry about edge cases like executing no instructions or handlingINSTRUMENTED_LINE
._PyExecutorObject
and_PyOptimizerObject
#108866