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-130704: Strength reduce `LOAD_FAST{_LOAD_FAST}` by mpage · Pull Request #130708 · python/cpython

Conversation

@mpage

@mpage mpage commented Feb 28, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

This PR eliminates most reference counting overhead for references pushed onto the operand stack using LOAD_FAST{_LOAD_FAST} when we can be sure that the reference in the frame outlives the reference that is pushed onto the operand stack. Instructions that meet this criteria are replaced with new variants (LOAD_FAST_BORROW{_LOAD_FAST_BORROW}) that push appropriately tagged borrowed references.

Performance on the benchmark suite looks good:

  • A ~3% improvement on the free-threaded build. I think this might actually be higher and will continue investigating, but I wanted to put this up for review.
  • A ~2.7% improvement on the default build.

This approach looks like its quite effective at optimization too, at least on the benchmark suite. Roughly 97% of LOAD_FAST{_LOAD_FAST} instructions are optimized according to pystats. Note that these stats were collected using fastbench, so may not match those collected using pyperformance exactly.

The main pieces of the PR are:

New bytecodes

This adds two new bytecode instructions: LOAD_FAST_BORROW and its superinstruction form, LOAD_FAST_BORROW_LOAD_FAST_BORROW.

A new optimization pass

This adds a new optimization pass, optimize_load_fast, to the bytecode compiler that identifies and optimizes eligible instructions. Please read the detailed comment in flowgraph.c for a description of how it works.

Runtime support changes

A new function, PyStackRef_Borrow, was added to the stackref API. It creates a new stackref from an existing stackref without incrementing the reference count on the underlying object.

There are a few places in the runtime where we need to convert borrowed references into owned references:

  • When a frame escapes into the heap (i.e. when it is copied into a generator or when its materialized and unwound).
  • When a reference flows up the call stack (i.e. in RETURN_VALUE or YIELD_VALUE).
  • When someone destroys a reference to the frame "out of band" by poking at f_locals. We place the old reference into a tuple owned by the frame object.

The default build also required:

  • Adding support for stackrefs in the GC.
  • Removing reuse of stackrefs in PyFloat_FromDoubleConsumeInputs.
  • Issue: Optimize reference counting overhead of LOAD_FAST variants #130704

📚 Documentation preview 📚: https://cpython-previews--130708.org.readthedocs.build/

Sorry, something went wrong.

Uh oh!

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

corona10 and Fidget-Spinner reacted with thumbs up emoji Fidget-Spinner and tekknolagi reacted with rocket emoji eendebakpt and Fidget-Spinner reacted with eyes emoji
All reactions
  • 👍 2 reactions
  • 🚀 2 reactions
  • 👀 2 reactions
mpage added 30 commits February 28, 2025 11:41
@mpage
Experiment with borrowing load_fast
d716faa
@mpage
Checkpoint poc
3736923
@mpage
Fix pyframe copy
7a14254
@mpage
Strengthen refs when frame is copied
b1607aa
@mpage
Cleanup
e765735
@mpage
Consider all instructions when computing mutations
291ace9
derp
@mpage
Add a super instruction
17d6dd6
@mpage
Don't optimize during quickening
0a74052
@mpage
Use abstract interpretation
afbfd88
@mpage
Fix test_generators
696c630
Ref will be 2 if borrowed
@mpage
Optimize returns
483ac7a
@mpage
Remove unused arg
259d5db
@mpage
Make sure we convert borrowed refs on frame
aeafa98
@mpage
Don't test with malformed bytecode
85f9a64
@mpage
Make sure we convert borrowed refs to func/code when copying generato…
b6ab2f7
…r frame
@mpage
Add support for disassembling LOAD_FAST_BORROW_LOAD_FAST_BORROW
fd1ad3d
@mpage
Make sure exc_obj is always defined
eee2195
Otherwise, it ends up being loaded using `LOAD_FAST_CHECK`, which increfs
and causes the refcount check to fail when it uses `LOAD_FAST_BORROW`.
@mpage
Make sure we store new stackrefs for frame executable/funcobj
d75ec9a
These need to be tagged appropriately, not just increfed, so that they are decrefed
when the frame is destroyed.
@mpage
Remove refcount check
66f5351
This may be 1 if the `LOAD_FAST` is optimized to a `LOAD_FAST_BORROW`. It's
not clear that this is testing anything useful, so remove it.
@mpage
Don't hardcode initial refcount in refcount tests
7ef6a0b
The initial value will differ depending on whether a owned or borrowed reference
is loaded onto the operand stack.
@mpage
Remove invalid bytecode from test_peepholer
2af2bbc
These don't push enough values on the stack.
@mpage
Fix invalid bytecode in `test_peepholer.DirectCfgOptimizerTests.test_…
bf19b7d
…unconditional_jump_threading`

Make sure we have a statically known stack depth
@mpage
Fix tests that checked for LOAD_FAST instructions that are now opti…
a9bca03
…mized to borrowed variants
@mpage
Update disassembly in test_dis to match new bytecode
293c317
@mpage
Fix refleak in _BINARY_OP_INPLACE_ADD_UNICODE
a12ccd9
PyStackRef_AsPyObjectSteal creates a new reference if the stackref
is deferred. This reference is leaked if we deopt before the corresponding
decref.
@mpage
Create new references to fast locals overwritten via f_locals
1ef26c5
These may provide support for borrowed references contained in frames
closer to the top of the call stack. Add them to a list attached to the
frame when they are overwritten, to be destroyed when the frame is destroyed.
@mpage
Implement two missing opcodes in the static analysis
1eb9226
`STORE_FAST_LOAD_FAST` and `LOAD_FAST_AND_CLEAR` both need to kill the
local.
@mpage
Use g_block_list when resetting stack depth
7291c49
This ensures we hit all the blocks
@mpage
Avoid reallocating state for each basic block
90bf8df
@mpage
Generators
9bfa922
mpage added 2 commits March 23, 2025 14:38
@mpage
Avoid having stackref only visible from the c stack
80fc5aa
@mpage
Merge branch 'main' into load-fast-borrow-absinterp
492cce1
@mpage

mpage commented Mar 24, 2025

Copy link
Copy Markdown
Contributor Author

A note for reviewers: I realized that we need to special case opcodes that leave some of their operands on the stack. This doesn't appear to reduce the effectiveness of the optimization. Stats look roughly the same as the previous version. Performance is a little worse (2.1% for the free-threaded build, 2.5% for the default), but it's hard for me to tell if that's just from noise in the runner or from changes that landed in main since the last time I ran the benchmarks.

All reactions

Sorry, something went wrong.

Uh oh!

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

@mpage
mpage requested review from colesbury, iritkatriel and markshannon March 24, 2025 18:19
colesbury
colesbury approved these changes Mar 24, 2025
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

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.

LGTM

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 change review labels Mar 24, 2025
markshannon
markshannon approved these changes Mar 26, 2025
View reviewed changes

@markshannon markshannon 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.

Missing one hint for the cases generator, otherwise looks good.

We could make the analysis more robust by using the cases generator, but that's for a later PR.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Python/bytecodes.c
_PyInterpreterFrame *gen_frame = &gen->gi_iframe;
STACK_SHRINK(1);
_PyFrame_StackPush(gen_frame, v);
_PyFrame_StackPush(gen_frame, PyStackRef_MakeHeapSafe(v));

@markshannon markshannon Mar 26, 2025

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 think you need a DEAD(v); here

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Python/bytecodes.c Resolved

Uh oh!

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

Comment thread Python/flowgraph.c
break;
}

// We treat opcodes that do not consume all of their inputs on

@markshannon markshannon Mar 26, 2025 •
edited
Loading

Uh oh!

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

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 approach seems fine for now, but the code generator knows exactly how many values are popped and consumed, as opposed to peek at.

We should add a _PyOpcode_num_peeked function, the we'd have consumed = _PyOpcode_num_popped() - _PyOpcode_num_peeked() which would be more robust.

Sorry, something went wrong.

Uh oh!

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

All reactions
@mpage
Merge branch 'main' into load-fast-borrow-absinterp
2e38f0d
@mpage mpage added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 31, 2025
@bedevere-bot

bedevere-bot commented Mar 31, 2025

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @mpage for commit 2e38f0d 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F130708%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 31, 2025
@mpage
Merge branch 'main' into load-fast-borrow-absinterp
2c55722
@mpage
mpage merged commit 053c285 into python:main Apr 1, 2025
@bedevere-app bedevere-app Bot removed the awaiting merge label Apr 1, 2025
@mpage mpage mentioned this pull request Apr 1, 2025
gh-131987: Bump the magic number #131991
Merged
@wjakob wjakob mentioned this pull request Apr 10, 2025
[BUG]: Python 3.14a7 breaks reference call policy test test50_call_policy() wjakob/nanobind#1006
Closed
jamadden added a commit to python-greenlet/greenlet that referenced this pull request Apr 11, 2025
@jamadden
Add initial support for Python 3.14a7.
faa28f4
- Header files have moved around.
- Reference counting has changed. It appears to be python/cpython#130708 that's eliding some reference counting within functions and caused us to need to lower our expected reference count in a few places. NOTE: I'm not 100% sure this is the case; dis.dis is broken and won't show the function bodies so I can't confirm the new opcodes are being used.
jamadden added a commit to python-greenlet/greenlet that referenced this pull request Apr 11, 2025
@jamadden
Add initial support for Python 3.14a7.
cbbe9d2
- Header files have moved around.
- Reference counting has changed. It appears to be python/cpython#130708 that's eliding some reference counting within functions and caused us to need to lower our expected reference count in a few places. NOTE: I'm not 100% sure this is the case; dis.dis is broken and won't show the function bodies so I can't confirm the new opcodes are being used.
jamadden added a commit to python-greenlet/greenlet that referenced this pull request Apr 11, 2025
@jamadden
Add initial support for Python 3.14a7.
87bf552
- Header files have moved around.
- Reference counting has changed. It appears to be python/cpython#130708 that's eliding some reference counting within functions and caused us to need to lower our expected reference count in a few places. NOTE: I'm not 100% sure this is the case; dis.dis is broken and won't show the function bodies so I can't confirm the new opcodes are being used.
jamadden added a commit to python-greenlet/greenlet that referenced this pull request Apr 11, 2025
@jamadden
Add initial support for Python 3.14a7.
cf7f854
- Header files have moved around.
- Reference counting has changed. It appears to be
  python/cpython#130708 that's eliding some
  reference counting within functions and caused us to need to lower
  our expected reference count in a few places. NOTE: I'm not 100%
  sure this is the case; but `dis.dis` shows the new opcode being used
  for the variables we're testing the refcount of.
@jamadden jamadden mentioned this pull request Apr 11, 2025
Add initial support for Python 3.14a7. python-greenlet/greenlet#442
Merged
@mpage mpage mentioned this pull request Apr 28, 2025
BUG: Python 3.14 may not increment refcount pandas-dev/pandas#61368
Closed
3 tasks done
@efimov-mikhail efimov-mikhail mentioned this pull request May 4, 2025
segfault when deleting an object after returning it #133371
Closed
@efimov-mikhail efimov-mikhail mentioned this pull request Sep 29, 2025
Use after free (stackref borrow after close) in BINARY_OP_INPLACE_ADD_UNICODE #130786
Closed
@tyler-yankee tyler-yankee mentioned this pull request Nov 3, 2025
[setup] Move macOS to Python 3.14 RobotLocomotion/drake#23655
Merged
@Fidget-Spinner Fidget-Spinner mentioned this pull request Jan 3, 2026
BINARY_OP_INPLACE_ADD_UNICODE has wrong reference count management #143401
Closed
navytux added a commit to navytux/pygolang that referenced this pull request Mar 18, 2026
@navytux
golang_str: tests: Fix test_strings_refcount for py3.14
b273884
Running that test on py3.14 fails:

    ____________________ test_strings_refcount _____________________

        def test_strings_refcount():
            # buffer with string data - not bytes nor unicode so that when builtin
            # string types are patched there is no case where bytes is created from the
            # same bytes, or unicode is created from the same unicode - only increasing
            # refcount of original object.
            data = bytearray([ord('a'), ord('b'), ord('c'), ord('4')])

            # first verify our logic on std type
            obj = bytes(data);      assert type(obj) is bytes
    >       gc.collect();   assert sys.getrefcount(obj) == 1+1   # +1 due to obj passed to getrefcount call
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    E       AssertionError: assert 1 == (1 + 1)
    E        +  where 1 = <built-in function getrefcount>(b'abc4')
    E        +    where <built-in function getrefcount> = sys.getrefcount

    golang/golang_str_test.py:301: AssertionError

That happens because on py3.14 the interpreter now does not add a
reference when passing on-stack argument to a function if it can prove
that is safe to do so.

Please see python/cpython#130708 and
python/cpython@f2379535 for details.

-> Adjust the test to account for that on py3.14 .

/reviewed-by @jerome
/reviewed-on https://lab.nexedi.com/nexedi/pygolang/-/merge_requests/39 + https://lab.nexedi.com/nexedi/slapos/-/merge_requests/1863#note_260064
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brandtbucher brandtbucher brandtbucher left review comments
@colesbury colesbury colesbury approved these changes
@markshannon markshannon markshannon approved these changes
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner Fidget-Spinner is a code owner
@DinoV DinoV Awaiting requested review from DinoV
@ncoghlan ncoghlan Awaiting requested review from ncoghlan ncoghlan is a code owner
@warsaw warsaw Awaiting requested review from warsaw warsaw is a code owner
@iritkatriel iritkatriel Awaiting requested review from iritkatriel iritkatriel is a code owner

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.

6 participants

@mpage @markshannon @colesbury @bedevere-bot @iritkatriel @brandtbucher
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.