-
-
Notifications
You must be signed in to change notification settings - Fork 33.8k
gh-141504: Factor out tracing and optimization heuristics into a single object #143381
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Python/pystate.c
Outdated
| _tstate->jit_metrics.side_exit_initial_value = SIDE_EXIT_INITIAL_VALUE; | ||
| _tstate->jit_metrics.side_exit_initial_backoff = SIDE_EXIT_INITIAL_BACKOFF; | ||
|
|
||
| char *env = Py_GETENV("PYTHON_JIT_JUMP_BACKWARD_INITIAL_VALUE"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may need to extract utility function to handle those things.
Include/internal/pycore_tstate.h
Outdated
| typedef struct _PyJitMetrics { | ||
| uint16_t jump_backward_initial_value; | ||
| uint16_t jump_backward_initial_backoff; | ||
| uint16_t side_exit_initial_value; | ||
| uint16_t side_exit_initial_backoff; | ||
| } _PyJitMetrics; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you rename this to _PyJitPolicy pleases?
Also please then wrap this into an overall struct called _PyPolicies. The reason is that we might want to add more structs later, like for the interpreter.
So it should be
| typedef struct _PyJitMetrics { | |
| uint16_t jump_backward_initial_value; | |
| uint16_t jump_backward_initial_backoff; | |
| uint16_t side_exit_initial_value; | |
| uint16_t side_exit_initial_backoff; | |
| } _PyJitMetrics; | |
| typedef struct _PyJitPolicy { | |
| uint16_t jump_backward_initial_value; | |
| uint16_t jump_backward_initial_backoff; | |
| uint16_t side_exit_initial_value; | |
| uint16_t side_exit_initial_backoff; | |
| } _PyJitPolicy; | |
| typedef struct _PyPolicy { | |
| _PyJitPolicy jit; | |
| } _PyPolicy; |
Python/specialize.c
Outdated
| PyThreadState *tstate = _PyThreadState_GET(); | ||
| _PyThreadStateImpl *tstate_impl = (_PyThreadStateImpl *)tstate; | ||
| jump_counter = initial_jump_backoff_counter( | ||
| tstate_impl->policy.jit.jump_backward_initial_value, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Fidget-Spinner
The crash is directly related to this line.
But ENABLE_SPECIALIZATION_FT does not mean TIER2 is enabled.
Do we need to move jump_backward_initial_value and jump_backward_initial_backoff to policy.interpreter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JUMP_BACKWARD_INITIAL_VALUE is only used by the JIT. On normal build/FT build, the CACHE of JUMP_BACKWARD is unused. So we can just keep it for the JIT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I realised the CI is now failing because FT doesn't build with JIT.
In that case, yes please move this to an interpreter policy.
Include/internal/pycore_tstate.h
Outdated
| #endif | ||
| #if _Py_TIER2 | ||
| _PyJitTracerState jit_tracer_state; | ||
| _PyPolicy policy; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be moved out of the ifdef. CI is failing.
Fidget-Spinner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments to cleanup the code. After this I will approve. Thanks!
Include/internal/pycore_backoff.h
Outdated
| #define JUMP_BACKWARD_INITIAL_BACKOFF 6 | ||
| static inline _Py_BackoffCounter | ||
| initial_jump_backoff_counter(void) | ||
| initial_jump_backoff_counter(uint16_t initial_value, uint16_t initial_backoff) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should take a single value _tstate/tstate and grab the jump backward values from it.
Include/internal/pycore_backoff.h
Outdated
|
|
||
| static inline _Py_BackoffCounter | ||
| initial_temperature_backoff_counter(void) | ||
| initial_temperature_backoff_counter(uint16_t initial_value, uint16_t initial_backoff) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
Python/pystate.c
Outdated
| } | ||
|
|
||
| static inline void | ||
| init_jit_metric(uint16_t *target, const char *env_name, uint16_t default_value, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to init_policy
Python/pystate.c
Outdated
| _tstate->asyncio_running_task = NULL; | ||
|
|
||
| // Initialize interpreter policy from environment variables | ||
| init_jit_metric(&_tstate->policy.interp.jump_backward_initial_value, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename here as well
Fidget-Spinner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. Thanks!
|
Note: the names are a little lengthy. But considering this is an internal CPython only env var used for testing, we can determine proper names later when/if we decide to expose them. |
Uh oh!
There was an error while loading. Please reload this page.