Uh oh!
There was an error while loading. Please reload this page.
gh-137838: Move _PyUOpInstruction buffer to PyInterpreterState - #138918
Conversation
…to _PyThreadStateImpl
| * uint16_t jump_target; | ||
| * uint16_t error_target; | ||
| */ | ||
| typedef struct _PyUOpInstruction{ |
There was a problem hiding this comment.
@Fidget-Spinner I move _PyUOpInstruction to here becasue of declaration issue. But we may need to move this struct for better place.
corona10
commented
Sep 15, 2025
I plan to add a new CI for nopt JIT with a separate PR. |
corona10
commented
Sep 15, 2025
And I fully checked that |
Uh oh!
There was an error while loading. Please reload this page.
Fidget-Spinner
left a comment
There was a problem hiding this comment.
Looks good, but just come over and ask Mark if he's okay with the struct being in pycore_optimizer.h.
corona10
commented
Sep 15, 2025
Now it's currently located in |
Fidget-Spinner
commented
Sep 15, 2025
Woops yes that's the one I was thinking of. Thanks. |
corona10
commented
Sep 15, 2025
Do you feel comfortable if I define |
Fidget-Spinner
commented
Sep 15, 2025
That works too. |
This reverts commit ab39a55.
This reverts commit 877f3a9.
corona10
commented
Sep 17, 2025
When I move to interpreter state, those tests are failed and could be race condition in some where, need to investgate with Windows machine. |
I still need to debug this in detail, but the buffer causes problems when it’s shared at the interpreter level under an asyncio workload. A per-thread approach also seems safe for the GIL build. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
corona10
commented
Sep 17, 2025
|
corona10
commented
Sep 17, 2025
Okay it turn out that it is not related to this change but looks like trigger other bug in asyncio. Let me test.. |
corona10
commented
Sep 17, 2025
@brandtbucher@Fidget-Spinner Fixed! Ready to be merged! |
| struct _stoptheworld_state stoptheworld; | ||
| struct _qsbr_shared qsbr; | ||
| struct _PyUOpInstruction *jit_uop_buffer; |
There was a problem hiding this comment.
The issue was that I declared this field only for the TIER2 build.
We might need to adjust it properly if we want to exclude non-JIT builds.
But since the field is only allocated when the TIER2 build is enabled, I think it’s fine to leave it as is.
markshannon
commented
Sep 17, 2025
Thanks for doing this. I have a few comments:
#defineUOP_MAX_TRACE_LENGTH JIT_TRACE_BUFFER_SIZE/sizeof(_PyUOpInstruction)
|
| #ifdef _Py_TIER2 | ||
| _Py_ClearExecutorDeletionList(interp); | ||
| if (interp->jit_uop_buffer != NULL) { | ||
| _PyObject_VirtualFree(interp->jit_uop_buffer, UOP_BUFFER_SIZE); |
There was a problem hiding this comment.
@brandtbucher _PyObject_VirtualFree doesn’t return a value, and the current CPython codebase doesn’t check whether it succeeds or not
corona10
commented
Sep 17, 2025
@markshannon Hopefully you will like the current change :) I 've applied most of the changes you requested! |
Uh oh!
There was an error while loading. Please reload this page.