gh-139109: JIT _EXIT_TRACE to ENTER_EXECUTOR rather than _DEOPT #141573
Conversation
Lib/test/test_capi/test_opt.py
Outdated
| # Inner loop warms up first. | ||
| # Outer loop warms up later, linking to the inner one. | ||
| # Therefore, at least two executors. | ||
| self.assertGreaterEqual(len(get_all_executors(f)), 2) |
There was a problem hiding this comment.
Could you please explain, why this test checks _DEOPT vs _EXIT_TRACE pair?
On my machine it passes both on this PR and on main branch.
Using option --enable-experimental-jit=interpreter or --enable-experimental-jit=yes also doesn't matter.
There was a problem hiding this comment.
Hmmm it shouldn't pass on main. Let me strengthen the test.
There was a problem hiding this comment.
This version of test fails on main with --enable-experimental-jit=interpreter or with --enable-experimental-jit=yes as expected.
|
Also, I've noticed related warning: Python/optimizer.c: In function ‘_PyJit_translate_single_bytecode_to_trace’:
Python/optimizer.c:600:11: warning: operand of ‘?:’ changes signedness from ‘int’ to ‘unsigned int’ due to unsignedness of other operand [-Wsign-compare]
600 | ? (int)(target_instr - _Py_INTERPRETER_TRAMPOLINE_INSTRUCTIONS_PTR)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
|
I'm going to merge this as it's a minor but important fix and I have an approval. Sorry Mark if you want to review it. We can make any further changes in your PR that comes with the switch-case for trace recording. |
|
JFTR, I don't know much about the perfomance implications of this PR. |
|
Well, I don't know the exact perf implications either. However, the implication is the following:
Meanwhile, _EXIT_TRACE links executors. So if we see an ENTER_EXECUTOR and end with an _EXIT_TRACE, we end up linking from the first executor to the executor at the exit. For Jitted code, the benefits of staying in jitted code without going from jit -> interpreter -> back to jit is enormous. It's one function call with lots of register moves. |
|
Yes, I understand the purpose of this change. Meanwhile, thanks for the detailed clarification. |
2 participants
This was responsible for the pretty big perf regression in the final benchmarks over the old JIT. It got missed in the flurry of commits and reviews at the end.