Skip to content
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

GH-118093: Remove invalidated executors from side exits #121885

Merged
merged 4 commits into from
Jul 24, 2024

Conversation

brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Jul 17, 2024

When we invalidate executors, they get removed from the bytecode, but not from side exits. This is unfortunate, since:

  • It keeps the objects alive.
  • We need to check for validity at the start of every trace. Every time we enter these executors via a side exit, we deopt immediately (0.4% of _START_EXECUTOR instructions fail right now).
  • It keeps us from projecting new traces for these side exits.

This changes the invalidation machinery to walk over all side exits on valid traces and wipe out affected executors when they are invalidated. It should be impossible to enter an invalid executor now.

As a related cleanup: _Py_Executors_InvalidateDependency currently does some gymnastics to do the right thing in the face of memory pressure. I've replaced this with a fallback path that just wipes out all executors, since this is correct, simpler, and arguably the right thing to do anyways in this rare case.

No perf impact, minor improvements to stats (1% fewer tier one instructions, 1% more uops executed, and slight increases in traces created and optimizations attempted with no change in number of traces executed).

@brandtbucher brandtbucher added performance Performance or resource usage skip news interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Jul 17, 2024
@brandtbucher brandtbucher self-assigned this Jul 17, 2024
@markshannon
Copy link
Member

I'm worried about performance here. If a program has ~10_000 traces, checking all of them to invalidate only a few is bad enough. Having to traverse all the exits in all the executors would be considerably worse.

If checking for validity at the start of every trace is too expensive, we can modify the trace when invalidated instead:

  • In the tier 2 interpreter, change _START_EXECUTOR to _EXECUTOR_INVALID which does the exit.
  • In the JIT, point to an exit stub, instead of the normal jitted code.

It keeps us from projecting new traces for these side exits.

Can you make an issue for this? This seems like a bug, rather than just a performance issue.
We could detach the executor from the exit when we enter it, if it is invalid. Much like the old _COLD_EXIT used to do, but in reverse. All side executors will need to know the index of the exit they are attached to, but that's only 1 extra byte.

Regarding performance:
If we have traces clean up after themselves when invalidated and avoid the check in _START_EXECUTOR, we can leave cleanup to the GC. By tracking the number of live traces and invalid traces, we can perform a cleanup scan during GC that should scale.

@brandtbucher
Copy link
Member Author

After our discussion: I'll punt on iterating over all side exits (we can save this for proper GC of cold traces). Instead, I'll just modify _EXIT_TRACE to check the validity of the executor we're about to jump into and remove it if it's invalid.

@markshannon
Copy link
Member

LGTM.

@brandtbucher brandtbucher merged commit 794546f into python:main Jul 24, 2024
55 checks passed
nohlson pushed a commit to nohlson/cpython that referenced this pull request Jul 24, 2024
nohlson pushed a commit to nohlson/cpython that referenced this pull request Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants