GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||
GeezerVison is showing github.com — gh-143421: Move `JitOptContext` from stack allocation to per-thread heap allocation to avoid stack overflow. by cocolato · Pull Request #143536 · python/cpython Conversation
|
|
Updated! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: Kumar Aditya <[email protected]>
|
Can we avoid unnecessary pointer chasing by embedding the array in the jit state struct, and allocate the entire struct together instead of having part of it embedded in the thread state, and part in a new allocation? ie. instead of typedef struct _PyJitOptState { struct _JitOptContext *opt_context; } _PyJitOptState; typedef struct _PyJitTracerState { _PyUOpInstruction *code_buffer; _PyJitOptState opt_state; _PyJitTracerInitialState initial_state; _PyJitTracerPreviousState prev_state; _PyJitTracerTranslatorState translator_state; } _PyJitTracerState; we would have typedef struct _PyJitTracerState { _PyJitTracerInitialState initial_state; _PyJitTracerPreviousState prev_state; _PyJitTracerTranslatorState translator_state; _JitOptContext opt_context; _PyUOpInstruction code_buffer[UOP_MAX_TRACE_LENGTH]; } _PyJitTracerState; Doing so would also decouple the thread state header from the JIT headers, as only a pointer to an opaque struct would need to be declared in the thread state header. I'm also concerned about the amount of allocation and freeing required to compile a small trace. Maybe allocate once and only free when the thread is destroyed? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
I can try implementing it this way, but I'd like to know if this should be done in a new PR/issue. Perhaps we could use benchmarks to determine potential performance issues from allocating |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Isnt that already being done? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@cocolato I think we can apply Mark's changes, it means we would need no new allocations anymore as its all heap allocated with the _PyThreadStateImpl struct. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
I'm not sure if the current implementation of the new |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Very cool, thank you!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
This caused a 100% slowdown in bench_thread_pool benchmark https://doesjitgobrrr.com/run/2026-01-08 We need to lazily allocate it and pointer chase, otherwise we're slowing down significantly the spawning of threads. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…o per-thread heap allocation (pythonGH-143536)" This reverts commit aeb3403.
There was an error while loading. Please reload this page.
So should we revert to the first version?4e7a918 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
I will have a PR up. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Sorry, I will run a local benchmark first before making important changes in the next time. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
No it's fine. That's the point of having a benchmark runner, to catch things like this! It's hard to predict what a change will affect, so that's the point of having benchark runners to save human time. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
PR up at #143597 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…read heap allocation (pythonGH-143536) * move JitOptContext to _PyThreadStateImpl * make _PyUOpInstruction buffer a part of _PyThreadStateImpl Co-authored-by: Kumar Aditya <[email protected]>
…read heap allocation (pythonGH-143536) * move JitOptContext to _PyThreadStateImpl * make _PyUOpInstruction buffer a part of _PyThreadStateImpl Co-authored-by: Kumar Aditya <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.