You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I don't know anything about the plan here, so this may not be news. :-) Couldn't you check for the needed stack space once at the beginning of the executor? And if there's a loop, lift it out of there. (And if it fits within the stack space of the code object already, drop it altogether.)
Makes sense. Let's go with consolidating them first then another follow-up PR for lifting. Lifting stuff out of loops requires _JUMP_ABSOLUTE and I can just point to the code from my old PR to copy. (Though the heuristics need someone else to work on it).
@gvanrossum I think @lazorchakp might be working on this, @lazorchakp do you mind if Guido takes over? If you have something with an implementation up but no tests that's okay, you can just submit a draft PR and Guido can probably take over if you don't mind.
Sorry everyone - I've been busier than anticipated and haven't made concrete progress on this yet. If it's alright though, I'd still like to try it, and I should be able to get a draft up tonight if that works. @Fidget-Spinner has given me some guidance over email, but I'd gladly accept as much mentoring as you both are willing to provide 🙂
That being said, @gvanrossum if you'd rather go for it yourself, that's quite alright! Just let me know.
My guess is that the critical trick should be to change the uop that checks for stack space to take a number indicating the amount of stack space needed, rather than digging that value out of the code object (which must be retrieved from the function object, which must be dug out of the stack at position -2-oparg, IIRC).
Maybe this number can be put in its operand field by the initial trace projection pass (which needs the code object anyway). Then a later dedicated pass can combine stack checks as long as they are within the same "scope" (i.e., _PUSH_FRAME/_POP_FRAME pair).
That sounds great. It makes sense to me that we'd now need _CHECK_STACK_SPACE to know up-front how much space it's looking for. I'll see if I can take your suggestion and modify the uop so that this information is available for the optimization pass.
I'm at the point where I have the operand for each _CHECK_STACK_SPACE being correctly assigned to its corresponding function's framesize (pushed a rough branch). Before I implement the actual optimization though, I want to make sure I'm reasoning about this correctly - I'm making some heavy assumptions about the underlying machinery. Are the following cases correct? (other ops removed for simplicity):
CASE 1 - sequential function calls:
_CHECK_STACK_SPACE A
_PUSH_FRAME
_POP_FRAME
_CHECK_STACK_SPACE B
_PUSH_FRAME
_POP_FRAME
proposed optimization:
_CHECK_STACK_SPACE max(A, B)
_PUSH_FRAME
_POP_FRAME
_PUSH_FRAME
_POP_FRAME
CASE 2 - nested function calls:
_CHECK_STACK_SPACE A
_PUSH_FRAME
_CHECK_STACK_SPACE B
_PUSH_FRAME
_POP_FRAME
_POP_FRAME
proposed optimization:
_CHECK_STACK_SPACE A + B
_PUSH_FRAME
_PUSH_FRAME
_POP_FRAME
_POP_FRAME
If these cases are correct, I should be able to combine everything into a single _CHECK_STACK_SPACE uop per trace, as @gvanrossum mentioned above.
Activity
lazorchakp commentedon Feb 29, 2024
cc @Fidget-Spinner
This might need some labels. I also looked around for a parent ticket, but didn't find anything particularly obvious
Fidget-Spinner commentedon Mar 1, 2024
I will link to #115419
gvanrossum commentedon Mar 3, 2024
I don't know anything about the plan here, so this may not be news. :-) Couldn't you check for the needed stack space once at the beginning of the executor? And if there's a loop, lift it out of there. (And if it fits within the stack space of the code object already, drop it altogether.)
Fidget-Spinner commentedon Mar 3, 2024
Makes sense. Let's go with consolidating them first then another follow-up PR for lifting. Lifting stuff out of loops requires
_JUMP_ABSOLUTEand I can just point to the code from my old PR to copy. (Though the heuristics need someone else to work on it).gvanrossum commentedon Mar 20, 2024
Is anyone working on this? Otherwise I'd like to give it a go.
Fidget-Spinner commentedon Mar 20, 2024
@gvanrossum I think @lazorchakp might be working on this, @lazorchakp do you mind if Guido takes over? If you have something with an implementation up but no tests that's okay, you can just submit a draft PR and Guido can probably take over if you don't mind.
gvanrossum commentedon Mar 20, 2024
I'd also be happy to mentor @lazorchakp !
lazorchakp commentedon Mar 20, 2024
Sorry everyone - I've been busier than anticipated and haven't made concrete progress on this yet. If it's alright though, I'd still like to try it, and I should be able to get a draft up tonight if that works. @Fidget-Spinner has given me some guidance over email, but I'd gladly accept as much mentoring as you both are willing to provide 🙂
That being said, @gvanrossum if you'd rather go for it yourself, that's quite alright! Just let me know.
gvanrossum commentedon Mar 20, 2024
No, I'd love to have you do it!
My guess is that the critical trick should be to change the uop that checks for stack space to take a number indicating the amount of stack space needed, rather than digging that value out of the code object (which must be retrieved from the function object, which must be dug out of the stack at position
-2-oparg, IIRC).Maybe this number can be put in its operand field by the initial trace projection pass (which needs the code object anyway). Then a later dedicated pass can combine stack checks as long as they are within the same "scope" (i.e.,
_PUSH_FRAME/_POP_FRAMEpair).lazorchakp commentedon Mar 20, 2024
That sounds great. It makes sense to me that we'd now need
_CHECK_STACK_SPACEto know up-front how much space it's looking for. I'll see if I can take your suggestion and modify the uop so that this information is available for the optimization pass.pythongh-116168 WIP: Remove extra _CHECK_STACK_SPACE uops
lazorchakp commentedon Mar 21, 2024
I'm at the point where I have the operand for each
_CHECK_STACK_SPACEbeing correctly assigned to its corresponding function's framesize (pushed a rough branch). Before I implement the actual optimization though, I want to make sure I'm reasoning about this correctly - I'm making some heavy assumptions about the underlying machinery. Are the following cases correct? (other ops removed for simplicity):CASE 1 - sequential function calls:
proposed optimization:
CASE 2 - nested function calls:
proposed optimization:
If these cases are correct, I should be able to combine everything into a single _CHECK_STACK_SPACE uop per trace, as @gvanrossum mentioned above.
13 remaining items
pythongh-116168: Remove extra _CHECK_STACK_SPACE uops
Merge remote-tracking branch 'upstream/main' into pythongh-116168-rem…
Merge remote-tracking branch 'upstream/main' into pythongh-116168-rem…
lazorchakp commentedon Mar 26, 2024
Still have to iterate with the pipeline a bit in my draft PR. I'll move it to Open and send an update here when it's officially ready for review.
Merge remote-tracking branch 'upstream/main' into pythongh-116168-rem…
Merge remote-tracking branch 'upstream/main' into pythongh-116168-rem…
lazorchakp commentedon Mar 28, 2024
Just opened #117242 for review
Merge remote-tracking branch 'upstream/main' into pythongh-116168-rem…
Merge remote-tracking branch 'upstream/main' into pythongh-116168-rem…
Merge remote-tracking branch 'upstream/main' into pythongh-116168-rem…
Merge remote-tracking branch 'upstream/main' into pythongh-116168-rem…
gh-116168: Remove extra `_CHECK_STACK_SPACE` uops (#117242)
gvanrossum commentedon Apr 4, 2024
This is done -- thanks again!
pythongh-116168: Remove extra `_CHECK_STACK_SPACE` uops (python#117242)