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-119866: Spill before escaping calls in all cases. by markshannon · Pull Request #124392 · python/cpython

Conversation

@markshannon

@markshannon markshannon commented Sep 23, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Also fixes #123391

This PR spills the stack pointer across escaping call, also writing the contents of the stack to memory where necessary.

How it works:

Adds a new Storage class which combines the state of the stack and the local variables within a micro-op.
Adds copying and merging functionality to that class, and the underlying stack and locals, to support tracking state across divergent flow.
Splits and merges the storage in if statements.
Tracks the state of conceptual stack and local variables, ensuring the necessary values are written to memory when needed.

The code generator needs to tell when inputs are dead, in order to known when the stack_pointer should be reduced when a call escapes. We can track explicit PyStackRef_CLOSE and DECREF_INPUTS, but many cases are implicit.
For these cases we add the DEAD macro to mark the variable as dead.

To simplify parsing, the code generator also enforces PEP 7 rules for braces.
A few of the changes in bytecodes.c are a result of this change.

Both interpreter performance and JIT performance show no slowdown.

Replaces #123397

  • Issue: Spill the stack pointer across calls in the interpreter. #119866

Sorry, something went wrong.

Uh oh!

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

All reactions
markshannon added 30 commits August 7, 2024 16:13
@markshannon
Add copy and == support to Stack class
75bda28
@markshannon
Merge branch 'main' into stack-copy-and-merge
c7e9102
@markshannon
blacken stack.py
d331371
@markshannon
Cases generator: Track reachability and divergent stacks in if statem…
4673d8a
…ents.
@markshannon
Fix type errors and rename ahead to look_ahead
132df06
@markshannon
Track state of output variables
b7f71d4
@markshannon
Handle stack and output locals togather as Storage class
b97dea9
@markshannon
Track locals as well as stack on differing paths
9838a05
@markshannon
Use 'PEP 7' in syntax error, to make it clear that this is not a C sy…
1f829be
…ntax error, just a tool limitation.
@markshannon
Push peeks back to stack in optimizer code gen
d2e5f12
@markshannon
Merge branch 'main' into stack-copy-and-merge
2753014
@markshannon
Update test
1754fc4
@markshannon
Cleanup whitespace
98f9720
@markshannon
Add tests for PEP 7 parsing and escaping call in condition
0284b3f
@markshannon
Remove merge artifact
03bea71
@markshannon
Remove test for escaping calls in conditions
ca2f457
@markshannon
Spill before escaping calls. Initial attempt
57de61f
@markshannon
Clean up asserts
a2e430a
@markshannon
Update test
95408d3
@markshannon
Flush locals as well on error
eb3a645
@markshannon
Spill stack on escaping calls. Preparatory work
00f5265
@markshannon
Spill stack contents on escaping calls
7de1e60
@markshannon
Find end of statement when anlyzing escaping calls
3bfed1b
@markshannon
Spill stack pointer as well. Work in progress
d7e1c82
@markshannon
Don't allow escaping calls in ERROR_IF or DEOPT_IF
2cc4f64
@markshannon
Improve tracking of stack and locals in conditional flow
8e258ad
@markshannon
Handle ERROR_NO_POP correctly
6da3fc6
@markshannon
Fix up handling of liveness
60ee3e9
@markshannon
Allow assignments of new refs to input as well as output variables.
9f2f3bb
@markshannon
Merge branch 'main' into spill-before-escaping-calls-2
87b5561
@markshannon
Insert extra braces to handle 'else if' without an 'else' correctly.
de3b86c
@markshannon

markshannon commented Oct 3, 2024

Copy link
Copy Markdown
Member Author

I've addressed all the issues.

I've increased the number of functions that we treat as non-escaping, but not every function mentioned above.

I think it safer to do this than to try to mark everything that is non-escaping and risk making a mistake, or any of those functions changing in the future such that it does escape.

Since the cost of spilling is low, spilling around such maybe-non-escaping calls has no measurable performance impact.

All reactions

Sorry, something went wrong.

Uh oh!

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

markshannon added 2 commits October 3, 2024 07:36
@markshannon
Merge branch 'main' into spill-before-escaping-calls-2
2db84df
@markshannon
Fix test. gc.get_referrers can now see values on generator stacks
a1e55d4
Fidget-Spinner
Fidget-Spinner approved these changes Oct 4, 2024
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'm going to approve this on the condition that usage of DEAD is properly documented in a follow-up PR. I don't think it's so clear when to use it correctly and when it might be misused.

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 Oct 4, 2024
@Fidget-Spinner

Fidget-Spinner commented Oct 4, 2024

Copy link
Copy Markdown
Member

Alternatively, you can add the documentation in, since you need to fix the failing test cases anyways.

All reactions

Sorry, something went wrong.

Uh oh!

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

@markshannon
markshannon requested review from 1st1, asvetlov, gvanrossum, kumaraditya303 and willingc as code owners October 4, 2024 14:22
@markshannon

markshannon commented Oct 4, 2024

Copy link
Copy Markdown
Member Author

I will leave the documentation of DEAD to another PR, as ERROR_IF and other macros are not documented either.
It will be easier to do them all together.

All reactions

Sorry, something went wrong.

Uh oh!

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

kumaraditya303
kumaraditya303 reviewed Oct 4, 2024
View reviewed changes
Comment thread Lib/test/test_asyncio/test_streams.py
self.assertListEqual(gc.get_referrers(exc), [])

asyncio.run(main())
self.assertListEqual(gc.get_referrers(exc), [main_coro])

@kumaraditya303 kumaraditya303 Oct 4, 2024

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.

Why this change?

Sorry, something went wrong.

Uh oh!

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

All reactions

@markshannon markshannon Oct 4, 2024

Copy link
Copy Markdown
Member Author

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.

Because we are spilling the stack pointer, so the GC can see the locals of the generator.

Sorry, something went wrong.

Uh oh!

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

All reactions
gvanrossum
gvanrossum reviewed Oct 4, 2024
View reviewed changes

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

The changes to asyncio look fine to me, but maybe a comment (along the lines of what you explained to Kumar) would be helpful.

Sorry, something went wrong.

Uh oh!

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

All reactions
brandtbucher
brandtbucher approved these changes Oct 5, 2024
View reviewed changes

@brandtbucher brandtbucher 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'm far from an expert on the cases generator, but I didn't see any obvious issues there. A few notes:

  • It should be an error if somebody uses a name after it has been killed by either DEAD(...) or INPUTS_DEAD().
  • It should be an error to kill a name twice in the same path.
  • Can you remind me why DEAD(...)/INPUTS_DEAD() needs to be explicit in the code, and can't just be inferred from the location of the last use? I vaguely remember discussing this at the sprint, but I forget why.
  • Can we handle scalars-under-arrays in the analyzer, so we don't have these awkward length-one arrays?

I'm approving this PR because I think it probably needs to happen and I trust that this is the best way of doing this. But I think in general, we might need to take a step back and consider the huge amount of complexity the has accumulated in bytecodes.c, and the thousands of lines of code in the cases generator that process it. The DSL was originally introduced to reduce the boilerplate and complexity in the interpreter loop... I think it still does a good job of this, as evidenced by this PR. But the mental load when reading and editing this file has crept up incrementally with time, and I'm worried that rate is accelerating. It's beyond the scope of this issue, but we should probably consider if the DSL should be reworked to better support the needs of the cases generator.

Sorry, something went wrong.

Uh oh!

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

All reactions
@brandtbucher

brandtbucher commented Oct 5, 2024

Copy link
Copy Markdown
Member

Thanks for tackling this, by the way. It was a big project and it's really cool to see it working correctly (the newly-failing test was neat to see).

All reactions

Sorry, something went wrong.

Uh oh!

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

@markshannon

markshannon commented Oct 7, 2024

Copy link
Copy Markdown
Member Author

Can you remind me why DEAD(...)/INPUTS_DEAD() needs to be explicit in the code, and can't just be inferred from the location of the last use? I vaguely remember discussing this at the sprint, but I forget why.

Because variables hold references to objects the last use doesn't kill the variable. PyStackRef_CLOSE() closes the reference and kills the variable.
However, for calls that consume references and immortal objects, the code generator needs to be told that the reference is dead with DEAD

All reactions

Sorry, something went wrong.

Uh oh!

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

@markshannon

markshannon commented Oct 7, 2024

Copy link
Copy Markdown
Member Author

Can we handle scalars-under-arrays in the analyzer, so we don't have these awkward length-one arrays?

Theoretically yes, but mixing scalars and arrays is awkward. We want to be able to move scalars into registers, but having gaps in the in-memory stack makes things complicated.

All reactions

Sorry, something went wrong.

Uh oh!

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

@markshannon

markshannon commented Oct 7, 2024

Copy link
Copy Markdown
Member Author

#125046 for the other issues.

All reactions

Sorry, something went wrong.

Uh oh!

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

@markshannon
markshannon merged commit da071fa into python:main Oct 7, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Oct 7, 2024
@mdboom mdboom mentioned this pull request Oct 11, 2024
Compiler crash on MSVC when building with JIT and PGO #125217
Closed
graingert added a commit to graingert/cpython that referenced this pull request Oct 14, 2024
@graingert
fix incompatability with pythongh-124392
79606d6
1st1 pushed a commit that referenced this pull request Oct 14, 2024
@graingert
gh-124958: Revert "gh-125472: Revert "gh-124958: fix asyncio.TaskGrou…
0b28ea4
…p and _PyFuture refcycles ... (#125486)

* Revert "gh-125472: Revert "gh-124958: fix asyncio.TaskGroup and _PyFuture refcycles (#12… (#125476)"

This reverts commit e99650b.

* fix incompatability with gh-124392
This was referenced Oct 16, 2024
[3.12] gh-124958: fix asyncio.TaskGroup and _PyFuture refcycles (#124959) #125466
Merged
[3.14] change in behaviour in gc.get_referrers(some_local) #125603
Open
This was referenced Nov 6, 2024
Mark all objects reachable from roots as live before doing main cyclic GC pass #126491
Open
JIT error stubs don't account for peeks #126222
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum left review comments
@picnixz picnixz picnixz left review comments
@kumaraditya303 kumaraditya303 kumaraditya303 left review comments
@Fidget-Spinner Fidget-Spinner Fidget-Spinner approved these changes
@brandtbucher brandtbucher brandtbucher approved these changes
@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner
@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner
@willingc willingc Awaiting requested review from willingc willingc is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Interpreter code generators need to be able to handle basic flow control

Uh oh!

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

6 participants

@markshannon @Fidget-Spinner @brandtbucher @gvanrossum @picnixz @kumaraditya303
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.