Skip to content

GH-112354: Initial implementation of warm up on exits and trace-stitching - #114142

Merged
markshannon merged 50 commits into
python:mainfrom
faster-cpython:cold-exits
Feb 20, 2024
Merged

GH-112354: Initial implementation of warm up on exits and trace-stitching#114142
markshannon merged 50 commits into
python:mainfrom
faster-cpython:cold-exits

Conversation

@markshannon

@markshannonmarkshannon commented Jan 16, 2024

Copy link
Copy Markdown
Member

Only works for boolean guards, not type guards, for now. And _EXIT_TRACE now.

Needs to be carefully documented, there's some subtlety here.

Exits are implemented as an array of records attached to the executors, and a fixed set of cold exit executors.
We can shrink the _PyExitData further, by moving the counters into a global table, but that's for another PR.

See faster-cpython/ideas#644 for a rough sketch of the design.

@bedevere-appbedevere-appBot mentioned this pull request Jan 16, 2024
@brandtbucher
brandtbucher self-requested a review January 16, 2024 21:26
@markshannon

Copy link
Copy Markdown
MemberAuthor

@markshannon
markshannon marked this pull request as ready for review January 17, 2024 15:55
@brandtbucher

Copy link
Copy Markdown
Member

Sorry, didn't get to a proper review of this today. I've been trying (and failing) to merge the JIT branch into this one without crashing.

Maybe we can chat "in person" later?

@brandtbucher

Copy link
Copy Markdown
Member

Okay, I was able to get it working-ish... but it's not pretty. We can go over it when we meet.

@markshannon

Copy link
Copy Markdown
MemberAuthor

@gvanrossumgvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a first, partial pass. I've tried to focus on things that confused me. I haven't fully reviewed optimizer.c yet, nor the _COLD_EXIT uop definition, so I'll focus on those when I find some time.

Maybe it would be useful to add TODO comments indicating things you're planning to tackle in the near future (subsequent PRs)? Like the counters.

Comment threadPython/tier2_engine.md Outdated
Comment threadPython/tier2_engine.md
Comment threadPython/tier2_engine.md Outdated
Comment threadPython/tier2_engine.md Outdated
Comment threadPython/tier2_engine.md Outdated
Comment threadPython/bytecodes.c
Comment threadPython/ceval.c
Comment threadPython/bytecodes.c Outdated
Comment threadPython/optimizer.c Outdated
Comment threadInclude/cpython/optimizer.h
@markshannon

Copy link
Copy Markdown
MemberAuthor

Or better, executor_blah, matching e.g. executor_clear.

I'd prefer to save that for another PR. Maybe the one that removes the CounterExecutor.

@markshannon

Copy link
Copy Markdown
MemberAuthor

FTR, the thresholds are likely to get completely changed soon.

We probably want to change the fixed thresholds to some sort of adaptive thresholds, and consider the T1 and T2 thresholds together so that specialization works correctly.

Comment threadInclude/internal/pycore_uop_metadata.h
Comment threadPython/tier2_engine.md Outdated
Comment threadPython/tier2_engine.md Outdated
Comment threadPython/tier2_redundancy_eliminator_cases.c.h
@gvanrossum

Copy link
Copy Markdown
Member

If you look at GitHub there’s still a missing cast somewhere.

@brandtbucherbrandtbucher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't look too closely at the cases generator or optimizer (I'm assuming there are other eyes on that).

  • I appreciate the writeup!
  • I like that the interpreter/JIT interfaces are a lot more unified on this iteration. I know you're not crazy about conditional macro stuff but I think it makes things a lot easier to follow in this case. Thanks.
  • Suggestions to remove bit of tricky code duplication in template.c.
  • A couple of questions about setting tstate->previous_executor.
  • A few other random questions/suggestions I had while reading through.
  • One thing I find sort of tricky to keep track of is the nuances of the first couple of instructions in the trace (when/where _START_EXECUTOR is added, when/where pointers to optimizers/executors are smuggled in as operands, etc). This might be worth cleaning up now or in the future, or at least writing up. Not sure.

Comment threadTools/jit/template.c Outdated
Comment threadTools/jit/template.c
Comment threadTools/jit/template.c Outdated
Comment threadTools/jit/template.c Outdated
Comment threadTools/jit/template.c Outdated
Comment threadPython/optimizer.c
Comment threadInclude/cpython/optimizer.h Outdated
Comment threadPython/tier2_engine.md Outdated
Comment threadPython/bytecodes.c
Comment threadTools/jit/template.c
@bedevere-app

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

Comment threadPython/optimizer.c Outdated
Comment threadModules/_testinternalcapi.c Outdated
Comment threadPython/tier2_engine.md Outdated
Comment threadPython/tier2_engine.md Outdated
Comment threadPython/tier2_engine.md
@encukou

Copy link
Copy Markdown
Member

The refleaks buildbot failed on test.test_capi.test_opt.TestUops.test_confidence_score. Likely causes: GH-114142, GH-115558, or GH-115688
I'll investigate later if it's not fixed; you probably have more context.

@markshannon

Copy link
Copy Markdown
MemberAuthor

I don't think there is a new bug here, but this PR exposes it. #115727

woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@markshannon
markshannon deleted the cold-exits branch August 6, 2024 10:17
LukasWoodtli pushed a commit to LukasWoodtli/cpython that referenced this pull request Jan 22, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@markshannon@brandtbucher@gvanrossum@encukou@mdboom@iritkatriel