gh-146031: Allow keeping specialization enabled when specifying eval frame function - #146032
Conversation
3adb88b to
3c76262
Compare
3c76262 to
f3b7b90
Compare
Yep, it's then up to the replacement interpreter to decide what it wants to do here. If they're just a wrapper than they probably don't want to set the flag. If they're actually interpreting the byte code than they are responsible for the implementation of the inlined calls and can dispatch to them in their own inlined interpreter. |
|
This seems reasonable. The performance impact should be zero for CPython (as the new flag is only checked if PEP 523 is active). |
| @@ -318,4 +318,7 @@ PyAPI_FUNC(_PyFrameEvalFunction) _PyInterpreterState_GetEvalFrameFunc( | |||
| PyInterpreterState *interp); | |||
| PyAPI_FUNC(void) _PyInterpreterState_SetEvalFrameFunc( | |||
There was a problem hiding this comment.
I don't think we want to change this API again. It break PyTorch every time we do so.
Maybe add another function to set the flag.
| JUMP_TO_LABEL(start_frame); \ | ||
| #define DISPATCH_INLINED(NEW_FRAME) \ | ||
| do { \ | ||
| assert(tstate->interp->eval_frame == NULL || \ |
There was a problem hiding this comment.
Can you use your IS_PEP523_HOOKED macro here?
| func_calls = [c for c in actual_calls if c == "func"] | ||
| self.assertEqual(len(func_calls), 0) | ||
|
|
||
| # But the normal interpreter loop still shouldn't be inlining things | ||
| func_outer_calls = [c for c in actual_calls if c == "func_outer"] | ||
| self.assertNotEqual(len(func_outer_calls), 0) |
There was a problem hiding this comment.
It is better to use the .count method of arrays here. I.e. self.assertEqual(actual_calls.count('func'), 0)
0ae647a to
2426751
Compare
markshannon
left a comment
markshannon
left a comment
There was a problem hiding this comment.
The docs need updating.
Is it worth adding a "what's new" entry, so that the PyTorch developers (and anyone else using PEP 523) are made aware of this?
Other than that, the code looks good.
|
|
||
|
|
||
| .. c:function:: void _PyInterpreterState_SetEvalFrameFunc(PyInterpreterState *interp, _PyFrameEvalFunction eval_frame) | ||
| .. c:function:: void _PyInterpreterState_SetEvalFrameFunc(PyInterpreterState *interp, _PyFrameEvalFunction eval_frame, int allow_specialization) |
There was a problem hiding this comment.
I think you forgot to revert this and the text below.
Don't forget to add the new _PyInterpreterState_SetEvalFrameAllowSpecialization function as well
2426751 to
1bd159d
Compare
… eval frame function (python#146032) Allow keeping specialization enabled when specifying eval frame function
3 participants
An eval frame evaluator doesn't necessarily want to disable specialization of Python -> Python calls... The custom eval frame can either hook the specializing functions its self or they can specify vectorcall on a function object which prevents the nested dispatch.
This adds an argument to
_PyInterpreterState_SetEvalFrameFuncwhich allows the specification of whether or not specialization should be allowed. Setting it to 1 will allow the specializer to optimize Python -> Python calls. It's up to the eval frame replacement to make sure that this will work for them.📚 Documentation preview 📚: https://cpython-previews--146032.org.readthedocs.build/