GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-135379: Top of stack caching for the JIT. by markshannon · Pull Request #135465 · python/cpython

Conversation

@markshannon

@markshannon markshannon commented Jun 13, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

The stats need fixing and the generated tables could be more compact, but it works.

  • Issue: Top-of-stack caching in the JIT #135379

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sergey-miryanov reacted with hooray emoji
All reactions
  • 🎉 1 reaction
@bedevere-app bedevere-app Bot mentioned this pull request Jun 13, 2025
Top-of-stack caching in the JIT #135379
Closed
Fidget-Spinner
Fidget-Spinner reviewed Jun 13, 2025
View reviewed changes

@Fidget-Spinner Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

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 is really cool. I'll do a full review soon enough.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Python/optimizer.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@markshannon
markshannon force-pushed the tier-2-tos-caching branch from 78489ea to 2850d72 Compare June 19, 2025 14:49
@markshannon

markshannon commented Jun 20, 2025

Copy link
Copy Markdown
Member Author

Performance is in the noise, but we would need a really big speed up of jitted code for it to be more than noise overall.

The nbody benchmark, which spends a lot of time in the JIT shows a 13-18% speedup, except on Mac where it shows no speedup.
I don't know why that would be as I think we are using stock LLVM for Mac, not the Apple compiler.

Fidget-Spinner reacted with rocket emoji
All reactions
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fidget-Spinner

Fidget-Spinner commented Jun 20, 2025

Copy link
Copy Markdown
Member

The nbody benchmark, which spends a lot of time in the JIT shows a 13-18% speedup, except on Mac where it shows no speedup. I don't know why that would be as I think we are using stock LLVM for Mac, not the Apple compiler.

Nice. We use Apple's Compiler for the interpreter, though the JIT uses stock LLVm. Thomas previously showed that the version of the Apple compiler we use is subject to huge fluctuations in performance due to a PGO bug.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon
markshannon marked this pull request as ready for review June 20, 2025 15:04
@markshannon
markshannon requested review from brandtbucher and savannahostrowski as code owners June 20, 2025 15:04
@bedevere-app bedevere-app Bot added the awaiting core review label Jun 20, 2025
Fidget-Spinner
Fidget-Spinner reviewed Jun 20, 2025
View reviewed changes
Comment thread Misc/NEWS.d/next/Core_and_Builtins/2025-06-20-16-03-59.gh-issue-135379.eDg89T.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Fidget-Spinner
Fidget-Spinner reviewed Jun 20, 2025
View reviewed changes

@Fidget-Spinner Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

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.

I need to review the cases generator later.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Misc/NEWS.d/next/Core_and_Builtins/2025-06-13-13-32-16.gh-issue-135379.pAxZgy.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/optimizer.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/optimizer.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/optimizer.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Tools/cases_generator/analyzer.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Tools/cases_generator/analyzer.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@Fidget-Spinner Fidget-Spinner mentioned this pull request Jun 23, 2025
gh-134584: Eliminate redundant refcounting from _CALL_TYPE_1 #135818
Merged
Fidget-Spinner
Fidget-Spinner reviewed Jun 24, 2025
View reviewed changes
Comment thread Tools/cases_generator/analyzer.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Fidget-Spinner
Fidget-Spinner reviewed Jun 24, 2025
View reviewed changes

@Fidget-Spinner Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

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.

Stylistic nits

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Tools/cases_generator/tier2_generator.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Tools/cases_generator/tier2_generator.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Tools/cases_generator/analyzer.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@markshannon

markshannon commented Jun 26, 2025

Copy link
Copy Markdown
Member Author

Stats show that the spill/reload uops are about 13% of the total, and that we aren't spilling and reloading much more than the minimum.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Fidget-Spinner
Fidget-Spinner approved these changes Jun 26, 2025
View reviewed changes

@Fidget-Spinner Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

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.

I was discussing with Brandt the other day, and I concluded we need this to implement decref elimination with lower maintenance burden.

For decref elimination, there are two ways:

  1. Manually handwrite/cases generate a version of the instruction that has no PyStackref_CLOSE/DECREF_INPUTS.
  2. Specialize for POP_TOP.

Option 2 is a lot more maintainable in the long run and doesn't involve any more cases generator magic like in 1. It's also less likely to introduce a bug, as again less cases generator magic. So I'm more inclined to 2.

Option 2. requires TOS caching, otherwise it won't pay off. So this PR is needed otherwise we're blocking other optimizations. Unless of course, you folks don't mind me exercising some cases generator magic again 😉 .

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Jun 26, 2025
efimov-mikhail
efimov-mikhail reviewed Jun 26, 2025
View reviewed changes
Comment thread Tools/cases_generator/analyzer.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@MEDX000 MEDX000 added this to MEDX-arabic-programme Jun 26, 2025
@github-project-automation github-project-automation Bot moved this to Todo in MEDX-arabic-programme Jun 26, 2025
@brandtbucher

brandtbucher commented Jun 26, 2025

Copy link
Copy Markdown
Member

I've mentioned this before, but I'm uncomfortable adding all of this additional code/complexity without more certain payoff. Personally, "13%-18% faster nbody on most platforms, and everything else in the noise" really just doesn't feel like enough. If this is really a good approach to register allocation, we should be able to see a general pattern of improvement for multiple JIT-heavy benchmarks. That could mean more tweaking of magic numbers, or a change in approach.

I understand that this should be faster. But the numbers don't show that it is, generally. At least not enough to justify the additional complexity.

Option 2. requires TOS caching, otherwise it won't pay off. So this PR is needed otherwise we're blocking other optimizations.

I'm not sure that's the case. Yesterday, we benchmarked this approach together:

  • Just decref elimination: 7% faster nbody
  • This PR: 13% faster nbody
  • Both PRs together: 9% faster nbody

So the results seem to be more subtle and interconnected than "they both make each other faster". If anything (based just on the numbers I've seen), decref elimination makes TOS caching slower, and we shouldn't use it to justify merging this PR.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fidget-Spinner

Fidget-Spinner commented Jun 26, 2025

Copy link
Copy Markdown
Member

@brandtbucher that branch uses decref elimination via Option 1, ie its not scalable at all for the whole interpreter unless you let me go ham with the DSL

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@brandtbucher

brandtbucher commented Jun 26, 2025

Copy link
Copy Markdown
Member

But the effect is the same, right? Decref elimination seems to interact poorly with this branch for some reason (it's not quite clear why yet).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fidget-Spinner

Fidget-Spinner commented Jun 26, 2025

Copy link
Copy Markdown
Member

I can't say for sure whether the effect is the same.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fidget-Spinner

Fidget-Spinner commented Jun 26, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

One suspicion I have from examining the nbody traces is that the decref elimination is not actually improving the spilling. The problem is that we have too few TOS registers, so it spills regardless of whether we decref eliminate or not (right before the _BINARY_OP_SUBSCR_LIST_INT instruction, for example). So the decref elimination is not doing anything to the TOS caching at the moment.

The problem however, is that we are not actually increasing any of the spills. The maximum number of spills should stay the same regardless of decref elimination or not. So the benchmark results are a little suspect to me.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fidget-Spinner

Fidget-Spinner commented Jun 26, 2025

Copy link
Copy Markdown
Member

One more reason why I'm a little suspicious of our benchmarks: the JIT performance fluctuates quite a bit. On the Meta runners, the JIT fluctuates 2% weekly https://github.com/facebookexperimental/free-threading-benchmarking

On the MS runner, it's slightly better at 1% weekly https://github.com/faster-cpython/benchmarking-public . However, we're basing off our decision on this which I can't say I trust fully.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fidget-Spinner

Fidget-Spinner commented Jun 26, 2025

Copy link
Copy Markdown
Member

@brandtbucher so I decided to build 4 versions of CPython on my system, with the following configs:

  1. Base bda1218 (the reference)
  2. Decref
  3. tos caching
  4. tos caching + decref

All of them are standard benchmarking builds, Ie PGO, LTO, JIT.

These are the results for nbody:

Mean +- std dev: [bench-base-bda121862] 105 ms +- 1 ms -> [bench-decref-bda121862] 102 ms +- 1 ms: 1.03x faster
Mean +- std dev: [bench-base-bda121862] 105 ms +- 1 ms -> [bench-tos-caching-bda121862] 91.1 ms +- 0.5 ms: 1.16x faster
Mean +- std dev: [bench-base-bda121862] 105 ms +- 1 ms -> [bench-tos-caching-decref-bda121862] 85.8 ms +- 0.4 ms: 1.23x faster

So we do indeed see TOS caching and decref elimination helping each other out and compounding on my system.

sergey-miryanov and AIdjis reacted with thumbs up emoji tomasr8, efimov-mikhail, and rafalp reacted with eyes emoji
All reactions
  • 👍 2 reactions
  • 👀 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fidget-Spinner

Fidget-Spinner commented Jul 3, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

The Meta benchmarking results concur with my own results above:

  • rebased (TOS caching) - https://github.com/facebookexperimental/free-threading-benchmarking/tree/main/results/bm-20250624-3.15.0a0-50081dc-JIT
  • rebased-mine (TOS caching + refcount for nbody) - https://github.com/facebookexperimental/free-threading-benchmarking/tree/main/results/bm-20250625-3.15.0a0-992ac08-JIT

Most importantly:
nbody on TOS caching only: 1.18x faster
nbody on TOS caching+refcount: 1.21x faster
So refcount indeed improves on top of TOS caching on the meta runners

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon
markshannon requested a review from diegorusso as a code owner July 21, 2025 09:46
@markshannon
markshannon requested review from ZeroIntensity and ericsnowcurrently as code owners August 22, 2025 07:28
@Fidget-Spinner

Fidget-Spinner commented Aug 22, 2025

Copy link
Copy Markdown
Member

The tail calling CI seems to be failing because homebrew changed where they install clang (yet again). Will put up a separate PR to fix that.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fidget-Spinner

Fidget-Spinner commented Aug 22, 2025

Copy link
Copy Markdown
Member

Ok, I fixed the macOS CI on main. Please pull the changes in.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon

markshannon commented Aug 22, 2025

Copy link
Copy Markdown
Member Author

I thought that caching through side exits would speed things up, but it looks like it slows things down a bit if anything.
https://github.com/faster-cpython/benchmarking-public/tree/main/results/bm-20250822-3.15.0a0-efe4628-JIT

So, I've reverted that change.

Will rerun the benchmarks, to confirm...

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon markshannon mentioned this pull request Oct 19, 2025
BINARY_OP_SUBSCR_GETITEM constituent uops are invalid #140277
Closed
@markshannon

markshannon commented Oct 19, 2025

Copy link
Copy Markdown
Member Author

I was hoping to get a clear cross-the-board speedup before merging this.
But as it also enables decref removal and fixes the problem of intermediate values on the stack during tracing (#140277) I think we should merge this soon and tweak it later.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

savannahostrowski
savannahostrowski approved these changes Nov 3, 2025
View reviewed changes

@savannahostrowski savannahostrowski left a comment

Copy link
Copy Markdown
Member

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.

Took a quick skim of this - very neat! Thanks for also including all the perf numbers in the discussion, which helps counteract some of the initial trepidation I had around the amount of new generated code. This lays pretty solid groundwork for future optimizations as well.

Just one comment about the type change in pycore_optimizer.h

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Include/internal/pycore_optimizer.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Fidget-Spinner
Fidget-Spinner requested changes Nov 9, 2025
View reviewed changes
Comment thread Tools/cases_generator/analyzer.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot added awaiting changes and removed awaiting merge labels Nov 9, 2025
@bedevere-app

bedevere-app Bot commented Nov 9, 2025

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fidget-Spinner

Fidget-Spinner commented Nov 10, 2025

Copy link
Copy Markdown
Member

Once we get Savannah's LLVM 21 PR in, we should experiment with setting the TOS cache size to 4. I observe a lot of spilling due to the loop iterator taking up some space.

savannahostrowski reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon

markshannon commented Nov 12, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

I think we will want to vary the cache depending on both hardware and operating system.
We will want to vary it for different hardware, due to differing numbers of available registers.
We will want to vary for operating system, as different OSes have different calling conventions, changing the number of available registers.

All that for later PR(s) though.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon
Tier 2 TOS caching
4fec38d
Uses three registers to cache values at the top of the evaluation stack
This significantly reduces memory traffic for smaller, more common uops.
@markshannon
markshannon force-pushed the tier-2-tos-caching branch from 7d47f13 to 4fec38d Compare December 10, 2025 17:03
@markshannon

markshannon commented Dec 10, 2025

Copy link
Copy Markdown
Member Author

Apologies for the force push. This needed manually rebasing after the changes to the JIT frontend.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon
Merge branch 'main' into tier-2-tos-caching
4b24c15
@markshannon
markshannon requested a review from Fidget-Spinner December 10, 2025 17:44
Fidget-Spinner
Fidget-Spinner approved these changes Dec 10, 2025
View reviewed changes

@Fidget-Spinner Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

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.

I've read the code and understood it mostly as I had to rebase it a few times on my own branch.

These are the latest benchmarking results for this branch https://github.com/facebookexperimental/free-threading-benchmarking/blob/main/results/bm-20251210-3.15.0a2%2B-4b24c15-JIT/bm-20251210-vultr-x86_64-faster%252dcpython-tier_2_tos_caching-3.15.0a2%2B-4b24c15-vs-base.md

The 19% nbody speedup is still there, along with a modest ~7% speedup for richards as well.

So this bodes very well for the JIT. Considering it will unlock future optimizations in decref elimination, which would be very hard without this, we should just go ahead and merge it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

savannahostrowski, Sacul0457, sergey-miryanov, Object905, and Zheaoli reacted with hooray emoji savannahostrowski, Sacul0457, chris-eibl, sergey-miryanov, noamcohen97, Zheaoli, and efimov-mikhail reacted with rocket emoji
All reactions
  • 🎉 5 reactions
  • 🚀 7 reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting changes labels Dec 10, 2025
@markshannon
Merge branch 'main' into tier-2-tos-caching
a54eca7
@markshannon
markshannon merged commit 469f191 into python:main Dec 11, 2025
78 checks passed

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot removed the awaiting merge label Dec 11, 2025
@github-project-automation github-project-automation Bot moved this from Todo to Done in MEDX-arabic-programme Dec 11, 2025
@noamcohen97 noamcohen97 mentioned this pull request Dec 11, 2025
gh-134584: Eliminate redundant refcounting from _CALL_TUPLE_1 #135860
Merged
fatelei pushed a commit to fatelei/cpython that referenced this pull request Dec 12, 2025
@markshannon @fatelei
pythonGH-135379: Top of stack caching for the JIT. (pythonGH-135465)
74f2dd5
Uses three registers to cache values at the top of the evaluation stack
This significantly reduces memory traffic for smaller, more common uops.
@Fidget-Spinner Fidget-Spinner mentioned this pull request Dec 16, 2025
JIT Planning for 3.15 and 3.16 #139038
Open
4 of 19 tasks
@KRRT7 KRRT7 mentioned this pull request May 28, 2026
Widen specialized int fast paths to full int64 range #150424
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@diegorusso diegorusso diegorusso left review comments
@colesbury colesbury colesbury left review comments
@efimov-mikhail efimov-mikhail efimov-mikhail left review comments
@savannahostrowski savannahostrowski savannahostrowski approved these changes
@Fidget-Spinner Fidget-Spinner Fidget-Spinner approved these changes
@brandtbucher brandtbucher Awaiting requested review from brandtbucher brandtbucher is a code owner
@tomasr8 tomasr8 Awaiting requested review from tomasr8 tomasr8 is a code owner
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently
@ZeroIntensity ZeroIntensity Awaiting requested review from ZeroIntensity

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@markshannon @Fidget-Spinner @brandtbucher @diegorusso @colesbury @efimov-mikhail @savannahostrowski
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.