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-118926: Spill deferred references to stack in cases generator by colesbury · Pull Request #122748 · python/cpython

Conversation

@colesbury

@colesbury colesbury commented Aug 6, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

This automatically spills the results from _PyStackRef_FromPyObjectNew to the in-memory stack so that the deferred references are visible to the GC before we make any possibly escaping call.

  • Issue: GC may have inconsistent view of stacktop with deferred references #118926

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@colesbury @Fidget-Spinner
pythongh-118926: Spill deferred references to stack in cases generator
70272dc 
This automatically spills the results from `_PyStackRef_FromPyObjectNew`
to the in-memory stack so that the deferred references are visible to
the GC before we make any possibly escaping call.

Co-authored-by: Ken Jin <[email protected]>
@colesbury colesbury added skip news topic-free-threading labels Aug 6, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Aug 6, 2024
GC may have inconsistent view of stacktop with deferred references #118926
Closed
colesbury added 2 commits August 6, 2024 18:30
@colesbury
Fix analysis error message
a2e60e4
@colesbury
Add missing type annotation
b8c83c2
@colesbury
colesbury marked this pull request as ready for review August 6, 2024 18:58
@colesbury
colesbury requested a review from markshannon as a code owner August 6, 2024 18:58
@bedevere-app bedevere-app Bot added the awaiting core review label Aug 6, 2024
@colesbury colesbury mentioned this pull request Aug 6, 2024
gh-118926: Deferred reference counting GC changes for free threading #121318
Closed
@colesbury

colesbury commented Aug 6, 2024

Copy link
Copy Markdown
Contributor Author

@markshannon, this is the spilling logic split out from:

  • gh-118926: Deferred reference counting GC changes for free threading #121318

I've updated it to handle the changes to the stack from #122286. This required tracking output local variables in the stack before emitting tokens for a uop.

I've also moved the analysis to analyzer.py.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@colesbury
colesbury requested a review from Fidget-Spinner August 6, 2024 19:03
markshannon
markshannon requested changes Aug 7, 2024
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.

The stack shouldn't have "outputs" (or inputs) as it should model the state of the stack only.
The outputs (or inputs) are properties of the uops and instructions.

That can be fixed by passing the output variables to flush_single_var instead of caching them on the stack.

Note:
I think the correct long term approach to handling stack spilling is to lazily spill when code escapes. But that involves considerably deeper analysis.
Although this PR doesn't do that, it doesn't prevent us from doing in future. So I am happy to merge it once the above issue is fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Tools/cases_generator/analyzer.py Outdated
if tkn.kind != "IDENTIFIER" or tkn.text != "PyStackRef_FromPyObjectNew":
continue

if node.block.tokens[idx-1].kind != "EQUALS":

@markshannon markshannon Aug 7, 2024 •
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.

I think this is OK, as the first token should be the opening brace. Could you add a comment as to why this is safe.

Sorry, something went wrong.

Uh oh!

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

@colesbury colesbury Aug 7, 2024

Copy link
Copy Markdown
Contributor 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.

I think that's right, but I'll add condition to ensure that idx != 0 to make it explicit.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Tools/cases_generator/stack.py Outdated
self.top_offset = StackOffset.empty()
self.base_offset = StackOffset.empty()
self.variables: list[Local] = []
self.outputs: list[Local] = []

@markshannon markshannon Aug 7, 2024

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 stack shouldn't have "outputs" (or inputs) as it should model the state of the stack.
The outputs (or inputs) are properties of the uops and instructions.

Sorry, something went wrong.

Uh oh!

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

@colesbury colesbury Aug 7, 2024

Copy link
Copy Markdown
Contributor 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.

I've removed this

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Tools/cases_generator/stack.py Outdated
self.base_offset.clear()
self.top_offset.clear()

def flush_single_var(self, out: CWriter, var_name: str, cast_type: str = "uintptr_t", extract_bits: bool = False) -> None:

@markshannon markshannon Aug 7, 2024

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.

Rather than caching outputs on the stack, can you pass outputs as a list[StackItem] argument?

Sorry, something went wrong.

Uh oh!

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

@colesbury colesbury Aug 7, 2024

Copy link
Copy Markdown
Contributor 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.

I added outputs as a list[StackItem]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Tools/cases_generator/optimizer_generator.py Outdated
if local.defined:
locals[local.name] = local
out.emit(stack.define_output_arrays(prototype.stack.outputs))
outputs: list[Local] = []

@markshannon markshannon Aug 7, 2024

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 optimizer generator doesn't need to track the state of locals, nor is the GC involved, so shouldn't change.

Sorry, something went wrong.

Uh oh!

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

@colesbury colesbury Aug 7, 2024

Copy link
Copy Markdown
Contributor 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.

I've reverted the changes to optimizer_generator.py

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 changes and removed awaiting core review labels Aug 7, 2024
@bedevere-app

bedevere-app Bot commented Aug 7, 2024

Copy link
Copy Markdown

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

And if you don't make the requested changes, you will be poked with soft cushions!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@colesbury
Remove 'outputs' from Stack
479ca70
@colesbury

colesbury commented Aug 7, 2024

Copy link
Copy Markdown
Contributor Author

@markshannon, would you please take another look at it.

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.
@bedevere-app bedevere-app Bot added awaiting change review and removed awaiting changes labels Aug 7, 2024
@bedevere-app

bedevere-app Bot commented Aug 7, 2024

Copy link
Copy Markdown

Thanks for making the requested changes!

@markshannon: please review the changes made to this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app
bedevere-app Bot requested a review from markshannon August 7, 2024 15:01
@colesbury
Fix type checking errors
ae1c018
markshannon
markshannon approved these changes Aug 7, 2024
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.

Thanks for bearing with me on this one.

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 Aug 7, 2024
@colesbury colesbury added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Aug 7, 2024
@bedevere-bot

bedevere-bot commented Aug 7, 2024

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @colesbury for commit ae1c018 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-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-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Aug 7, 2024
@colesbury
colesbury merged commit 3e753c6 into python:main Aug 7, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Aug 7, 2024
@colesbury
colesbury deleted the gh-118926-flush-v2 branch August 7, 2024 17:23
blhsing pushed a commit to blhsing/cpython that referenced this pull request Aug 22, 2024
@colesbury @Fidget-Spinner @blhsing
pythongh-118926: Spill deferred references to stack in cases generator (
6431127 
python#122748)

This automatically spills the results from `_PyStackRef_FromPyObjectNew`
to the in-memory stack so that the deferred references are visible to
the GC before we make any possibly escaping call.

Co-authored-by: Ken Jin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannon markshannon markshannon approved these changes
@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner

Uh oh!

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

Assignees

No one assigned

Labels

skip news topic-free-threading

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.

3 participants

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