GH-113710: Add a "globals to constants" pass #114592
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
|
Hey @carljm, could you review the changes to the dict watcher here please? I've never seen that code before (sorry). |
gvanrossum
reviewed
Jan 28, 2024
carljm
reviewed
Jan 29, 2024
|
The JIT tests are failing. We should probably force enable uops and hammer the test suite. |
mdboom
reviewed
Feb 1, 2024
| @@ -609,7 +610,11 @@ init_interp_create_gil(PyThreadState *tstate, int gil) | |||
| static int | |||
| builtins_dict_watcher(PyDict_WatchEvent event, PyObject *dict, PyObject *key, PyObject *new_value) | |||
| { | |||
| RARE_EVENT_INC(builtin_dict); | |||
| PyInterpreterState *interp = _PyInterpreterState_GET(); | |||
| if (event != PyDict_EVENT_CLONED && interp->rare_events.builtin_dict < _Py_MAX_ALLOWED_BUILTINS_MODIFICATIONS) { | |||
There was a problem hiding this comment.
Maybe I'm just misunderstanding, but shouldn't this be >? As in, if we've modified builtins too many times, we invalidate everything?
Also, what is the purpose of excluding PyDict_EVENT_CLONED here?
There was a problem hiding this comment.
Maybe I'm just misunderstanding, but shouldn't this be >? As in, if we've modified builtins too many times, we invalidate everything?
Every time the builtins changes any executors that depend on it not changing must be invalidated.
So we invalidate the executor(s) every time it the builtins changes.
Upon reaching the threshold we no longer perform those optimizations, so the executors no longer depend on the builtins not changing, and we no longer need to invalidate.
Also, what is the purpose of excluding PyDict_EVENT_CLONED here?
Cloning doesn't modify the dict, so we don't care about it.
There was a problem hiding this comment.
@carljm OOI, why does Cinder care about dict cloning?
There was a problem hiding this comment.
PyDict_EVENT_CLONED is probably a bad name, because grammatically it implies the subject dict was cloned. What it actually means is that the subject dict was empty, and another dict was cloned into it, via e.g. .update(). So it does represent a change in the subject dict; all dict watcher events do.
I think it is probably irrelevant here, since it is (at least in practice) impossible(?) for builtins or module globals to be empty when code is running.
And I don't think Cinder cares about it in practice either, for that same reason, it was just for completeness in the dict watcher implementation, since all changes to a dict should be notified by dict watchers.
mdboom
reviewed
Feb 1, 2024
|
|
||
| if (PyDict_Watch(interp->rare_events.builtins_dict_watcher_id, interp->builtins) != 0) { | ||
| interp->dict_state.watchers[0] = &builtins_dict_watcher; |
There was a problem hiding this comment.
Since the first two slots are "reserved", it might be clearer to have #defines for these slots. e.g. BUILTINS_DICT_WATCHER_ID for 0 here.
There was a problem hiding this comment.
What is the rationale for bypassing PyDict_AddWatcher here and special-casing? Just a small amount of startup performance? Here, consistency and encapsulating management of the dict_state.watchers array might be better maintenance-wise.
There was a problem hiding this comment.
It keeps things simpler if we are guaranteed to have IDs. Otherwise we need to worry about what happens if we can't add either or both of the watchers.
|
Confirmed locally that the same failures happen with tier two (not just the JIT). |
|
|
Just curious: why is the error optimizer being called now? |
Maybe we aren't updating a threshold when we change the optimizer? |
mdboom
pushed a commit
to mdboom/cpython
that referenced
this pull request
Feb 7, 2024
Converts specializations of `LOAD_GLOBAL` into constants during tier 2 optimization.
aisk
pushed a commit
to aisk/cpython
that referenced
this pull request
Feb 11, 2024
Converts specializations of `LOAD_GLOBAL` into constants during tier 2 optimization.
fsc-eriker
pushed a commit
to fsc-eriker/cpython
that referenced
this pull request
Feb 14, 2024
Converts specializations of `LOAD_GLOBAL` into constants during tier 2 optimization.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a tier 2 optimizer pass that converts the micro-ops for loading globals and builtins to constants.
This should have two benefits:
Benchmarking and stats show a ~1% speedup, but it is the stats that are interesting.
The tier 2 stats show that we have replaced 3 billion guards with 1.2 cheaper guards, and that all
LOAD_GLOBAL_MODULEandLOAD_GLOBAL_BUILTINShave been replaced with inline constants.There are some changes to the tier 1 stats, which I think are a result of not optimizing when the global/builtin keys version doesn't match, so we avoid poor optimization. This drops us back into tier 1 for later re-optimization to tier 2, hopefully when the set of global variables has stabilized.
This shows up in the tier 1 stats and optimization attempts.
What this suggests is that we should be de-optimizing faster in tier 1 in this case, as once the keys version has changed it will never go back to the original value. But that's for another PR.