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-100762: Don't call `gen.throw()` in `gen.close()`, unless necessary. by markshannon · Pull Request #101013 · python/cpython

Conversation

@markshannon

@markshannon markshannon commented Jan 13, 2023 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Change oparg of YIELD_VALUE to hold the exception handler depth.
We can then test that in gen.close() to avoid calling gen.throw() unless necessary.

  • Issue: Generator finalization is slower in 3.11 vs 3.10 #100762

Sorry, something went wrong.

Uh oh!

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

All reactions
markshannon added 2 commits January 13, 2023 12:44
@markshannon
Store exception stack depth in YIELD_VALUE's oparg and use it avoid e…
5b081f2
…xpensive gen.throw() in gen.close() where possible.
@markshannon
Add news
6ec191a
@markshannon
markshannon requested a review from brandtbucher January 13, 2023 14:37
@markshannon
markshannon requested a review from iritkatriel as a code owner January 13, 2023 14:37
@bedevere-bot bedevere-bot added the awaiting core review label Jan 13, 2023
@bedevere-bot bedevere-bot mentioned this pull request Jan 13, 2023
Generator finalization is slower in 3.11 vs 3.10 #100762
Closed
@markshannon
Don't skip throwing if sub-generator raises.
044f2cf
@markshannon

markshannon commented Jan 13, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

Using the benchmarks in #101011 (comment), but using timeit instead of pyperf and a non-pgo and no-lto build because I'm impatient.
I get no significant change on "comprehension", "generator exhaust", and "all", but runtime is about 60% for "generator" and "all with comparison"

All reactions

Sorry, something went wrong.

Uh oh!

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

brandtbucher
brandtbucher approved these changes Jan 18, 2023
View reviewed changes
Comment thread Lib/importlib/_bootstrap_external.py Resolved

Uh oh!

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

@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Jan 18, 2023
gvanrossum
gvanrossum reviewed Jan 18, 2023
View reviewed changes
Comment thread Doc/library/dis.rst Resolved

Uh oh!

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

markshannon added 2 commits January 23, 2023 16:47
@markshannon
Merge branch 'main' into faster-gen-close
720f241
@markshannon
Restore note about 3.11 change.
dfd884e
gvanrossum
gvanrossum reviewed Jan 23, 2023
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.

I guess this will not be easy to backport, as we won't want to change the meaning of oparg in 3.11? Then again it appears to be unused except for the assert. Maybe it should just be a bit indicating whether it's safe to optimize out the throw?

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Objects/genobject.c Resolved

Uh oh!

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

Comment thread Python/compile.c Resolved

Uh oh!

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

Comment thread Objects/genobject.c Resolved

Uh oh!

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

Comment thread Lib/importlib/_bootstrap_external.py Resolved

Uh oh!

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

@brandtbucher

brandtbucher commented Jan 24, 2023

Copy link
Copy Markdown
Member

I guess this will not be easy to backport, as we won't want to change the meaning of oparg in 3.11?

I think a similar change in 3.11 would actually be simpler: just check if an exception table exists. If not, don't throw.

The only reason we need the YIELD_VALUE oparg in this 3.12 patch is because every generator now has an implicit try/except around the entire body, so it's a bit trickier to figure out if we're in that handler (which we can safely ignore) or something else.

All reactions

Sorry, something went wrong.

Uh oh!

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

@gvanrossum

gvanrossum commented Jan 24, 2023

Copy link
Copy Markdown
Member

Great, it'll just be different. :-)

brandtbucher 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 added 2 commits January 24, 2023 15:53
@markshannon
Update metadata
5b6eb20
@markshannon
Fix typo
f5ac751
@markshannon markshannon added the needs backport to 3.11 only security fixes label Jan 24, 2023
@markshannon
markshannon merged commit f02fa64 into python:main Jan 24, 2023
@miss-islington

miss-islington commented Jan 24, 2023

Copy link
Copy Markdown
Contributor

Thanks @markshannon for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the awaiting merge label Jan 24, 2023
@miss-islington

miss-islington commented Jan 24, 2023

Copy link
Copy Markdown
Contributor

Sorry, @markshannon, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker f02fa64bf2d03ef7a28650c164e17a5fb5d8543d 3.11

All reactions

Sorry, something went wrong.

Uh oh!

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

@miss-islington miss-islington assigned markshannon Jan 24, 2023
@brandtbucher

brandtbucher commented Jan 24, 2023

Copy link
Copy Markdown
Member

I don’t think this should be backported as-is, right? (See my comment above.)

All reactions

Sorry, something went wrong.

Uh oh!

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

@markshannon

markshannon commented Jan 25, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

@brandtbucher agreed.
The changes to genobject.c need backporting (with the change you suggest for determining when to skip the call to gen.throw(), but the other changes are 3.12 specific.

All reactions

Sorry, something went wrong.

Uh oh!

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

@markshannon markshannon mentioned this pull request Jan 25, 2023
[3.11] GH-100762: Don't call gen.throw() in gen.close(), unless necessary #101316
Closed
@markshannon
markshannon deleted the faster-gen-close branch September 26, 2023 12:55
@ZeroIntensity ZeroIntensity removed the needs backport to 3.11 only security fixes label Feb 17, 2025
@colesbury colesbury mentioned this pull request Dec 15, 2025
generator.close() doesn't clear frame on newly created generators #142766
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
@brandtbucher brandtbucher brandtbucher approved these changes
@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

@markshannon markshannon

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 @brandtbucher @gvanrossum @miss-islington @iritkatriel @bedevere-bot @ZeroIntensity
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.