GH-117086: Fix function version numbering #117093
Conversation
|
I need to add checks for builtins. |
| PyCodeObject *code = (PyCodeObject *)codeobj; | ||
| if (func_obj->func_builtins != tstate->interp->builtins) { | ||
| _PyFunction_SetVersion(func_obj, 0); | ||
| } | ||
| else if (code->co_version == 0) { | ||
| code->co_version = tstate->interp->func_state.next_version; | ||
| if (tstate->interp->func_state.next_version != 0) { | ||
| tstate->interp->func_state.next_version++; | ||
| } | ||
| assert(code->co_expected_globals_version == 0); | ||
| if (PyDict_CheckExact(GLOBALS())) { | ||
| code->co_expected_globals_version = ((PyDictObject *)GLOBALS())->ma_version_tag; | ||
| } | ||
| _PyFunction_SetVersion(func_obj, code->co_version); | ||
| } | ||
| else { | ||
| if (PyDict_CheckExact(GLOBALS()) && | ||
| code->co_expected_globals_version == ((PyDictObject *)GLOBALS())->ma_version_tag) { | ||
| _PyFunction_SetVersion(func_obj, code->co_version); | ||
| } | ||
| } | ||
| func = (PyObject *)func_obj; | ||
| Py_DECREF(codeobj); |
There was a problem hiding this comment.
This feels like an awfully large block of code for the JIT. Maybe it should be moved to a helper function?
Also, my brain is too small to fully grasp what will happen if we have a local function that references a global and is repeatedly created (maybe a generator expression inside a hot loop), when the referenced global changes (or some other global, given how dict versions work IIRC). Is that going to completely disable tier 2 for that hot loop, or is it just going to use _LOAD_GLOBAL_MODULE instead of _LOAD_CONST_INLINE?
A variant of the new test may reveal what happens more easily than trying to reason it through...
There was a problem hiding this comment.
This feels like an awfully large block of code for the JIT. Maybe it should be moved to a helper function?
Maybe, but that is the responsibility of the JIT builder. Let's not complicate the interpreter specification based on the current behavior of the JIT.
Is that going to completely disable tier 2 for that hot loop?
If the globals are changed, then we will get a different function version number.
That will disable some tier 2 optimizations. There is no reason why it should completely disable tier 2 optimization.
There was a problem hiding this comment.
I would like to play with this to see what actually happens. As I said, my brain is too small to reason it through, and your response ("should") does not allay my worries.
| PyCodeObject *code = (PyCodeObject *)func_obj->func_code; | ||
| if (code->co_expected_number_of_defaults < 0) { | ||
| code->co_expected_number_of_defaults = (int32_t)PyTuple_GET_SIZE(attr); | ||
| } | ||
| else if (code->co_expected_number_of_defaults != PyTuple_GET_SIZE(attr)) { | ||
| func_obj->func_version = 0; | ||
| } |
There was a problem hiding this comment.
This works, but couldn't we let the bytecode compiler give us the number? Again, I worry a bit about the size of the JIT code.
| @@ -78,6 +79,40 @@ def func(x=0): | |||
| ) | |||
|
|
|||
|
|
|||
| class TestOptimizations(unittest.TestCase): | |||
|
|
|||
| def test_globals_to_consts(self): | |||
There was a problem hiding this comment.
Note that this test only fails when tier 2 is selected (e.g. -X uops, or when the JIT is enabled).
We have machinery in test_capi/test_opt.py to force the optimizer on. It feels pretty arbitrary whether this test belongs here or there. Can you explain the criteria for where a tier-2-related test should go?
There was a problem hiding this comment.
Can you explain the criteria for where a tier-2-related test should go?
I don't think there are any. This test should pass for tier-1 and tier-2, as should all tests.
There was a problem hiding this comment.
Okay. In my mind this test should be moved to test_capi/test_opt.py and should be wrapped in with temporary_optimizer(_testinternalcapi.new_uop_optimizer()):. Otherwise it doesn't test anything unless build with the JIT or run with -Xuops.
Tracks not only the code object, but the globals and number of defaults when issuing function version numbers.
Fixes #117051