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-102192: Replace PyErr_Fetch/Restore etc by more efficient alternatives (in Objects/) by iritkatriel · Pull Request #102218 · python/cpython

Conversation

@iritkatriel

@iritkatriel iritkatriel commented Feb 24, 2023 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

(splitting up #102193 to make it more manageable).

  • Issue: Replace Fetch/Restore etc by the new exception APIs #102192

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
iritkatriel added 8 commits February 23, 2023 14:48
@iritkatriel
Objects/dictobject.c
f06d53a
@iritkatriel
Objects/frameobject.c
d5072bd
@iritkatriel
Objects/genobject.c
c40fc27
@iritkatriel
Objects/object.c
13cb48c
@iritkatriel
Objects/odictobject.c
fde54d4
@iritkatriel
Objects/typeobject.c
8fd66a8
@iritkatriel
Objects/weakrefobject.c
d83af14
@iritkatriel
Objects/exceptions.c
0298a04
@iritkatriel
iritkatriel requested review from markshannon and methane as code owners February 24, 2023 16:10
@bedevere-bot bedevere-bot added the awaiting core review label Feb 24, 2023
@bedevere-bot bedevere-bot mentioned this pull request Feb 24, 2023
Replace Fetch/Restore etc by the new exception APIs #102192
Closed
@iritkatriel
put back the Py_DECREF(exc)
09f2ab6
@iritkatriel
iritkatriel requested a review from erlend-aasland February 24, 2023 16:14
@iritkatriel iritkatriel added skip news 🔨 test-with-buildbots Test PR w/ buildbots; report in status section labels Feb 24, 2023
@bedevere-bot

bedevere-bot commented Feb 24, 2023

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 09f2ab6 🤖

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 Feb 24, 2023
@iritkatriel
Merge branch 'main' into fetch-restore-objects
0fef293
@iritkatriel iritkatriel added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 24, 2023
@bedevere-bot

bedevere-bot commented Feb 24, 2023

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 0fef293 🤖

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 Feb 24, 2023
@iritkatriel
Merge branch 'main' into fetch-restore-objects
9b5b62a
@iritkatriel iritkatriel added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 25, 2023
@bedevere-bot

bedevere-bot commented Feb 25, 2023

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 9b5b62a 🤖

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 Feb 25, 2023
@iritkatriel
Merge branch 'main' into fetch-restore-objects
6378d56
@iritkatriel iritkatriel added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Feb 26, 2023
@bedevere-bot

bedevere-bot commented Feb 26, 2023

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 6378d56 🤖

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 Feb 26, 2023
@iritkatriel iritkatriel added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Mar 2, 2023
@bedevere-bot

bedevere-bot commented Mar 2, 2023

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 0863f35 🤖

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 Mar 2, 2023
iritkatriel added 2 commits March 2, 2023 14:28
@iritkatriel
Merge branch 'main' into fetch-restore-objects
3f8b452
@iritkatriel
Merge branch 'main' into fetch-restore-objects
7a436c9
@iritkatriel iritkatriel added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Mar 2, 2023
@bedevere-bot

bedevere-bot commented Mar 2, 2023

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 7a436c9 🤖

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 Mar 2, 2023
@iritkatriel

iritkatriel commented Mar 3, 2023

Copy link
Copy Markdown
Member Author

All the refleaks tests passed now.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
markshannon
markshannon requested changes Mar 3, 2023
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.

Nice improvements.
There is some more redundant code that should also be removed (I think).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/exceptions.c
caught_type = (PyTypeObject *)exc;
PyObject *exc = PyErr_GetRaisedException();
PyTypeObject *caught_type = Py_TYPE(exc);
/* Ensure type info indicates no extra state is stored at the C level

@markshannon markshannon Mar 3, 2023

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.

Does all this stuff about "wrapping safely" make sense any more?
Since there are no "denomralized" exceptions, this should always work.
I think all this code is making sure that calling PyErr_NormalizeException is safe.

Sorry, something went wrong.

Uh oh!

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

@iritkatriel iritkatriel Mar 3, 2023

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.

Good question. Let me try to understand what's going on here.

Sorry, something went wrong.

Uh oh!

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

@iritkatriel iritkatriel Mar 3, 2023

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.

I don't think this is about PyErr_NormalizeException. This function is called from only one place:

/* Helper that tries to ensure the reported exception chain indicates the
 * codec that was invoked to trigger the failure without changing the type
 * of the exception raised.       
 */
static void
wrap_codec_error(const char *operation,
                 const char *encoding)
{
    /* TrySetFromCause will replace the active exception with a suitably
     * updated clone if it can, otherwise it will leave the original
     * exception alone.
     */
    _PyErr_TrySetFromCause("%s with '%s' codec failed",
                           operation, encoding);
}

This is a classic use case for notes (PEP 678). It is trying to create an exception of the same type with additional info (and does a lot of stuff to check that it can indeed safely create a new exception of the same type).

I would just rewrite this whole thing to attach the additional info as a note (but not in this PR, it's unrelated).

Sorry, something went wrong.

Uh oh!

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

@iritkatriel iritkatriel Mar 6, 2023

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.

A PR for that is at: #102407

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/exceptions.c
PyErr_SetRaisedException(exc);
return NULL;
}

@markshannon markshannon Mar 3, 2023

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 block comment should go as well (if I'm correct about removing the above code)

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 Objects/genobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/object.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting core review labels Mar 3, 2023
@bedevere-bot

bedevere-bot commented Mar 3, 2023

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.
@iritkatriel
remove redundant null check and obsolete comments (Mark's review comm…
9b68505 
…ents)
@iritkatriel

iritkatriel commented Mar 3, 2023

Copy link
Copy Markdown
Member Author

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-bot bedevere-bot added awaiting change review and removed awaiting changes labels Mar 3, 2023
@bedevere-bot

bedevere-bot commented Mar 3, 2023

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.
@iritkatriel
Merge branch 'main' into fetch-restore-objects
c23baa2
@bedevere-bot
bedevere-bot requested a review from markshannon March 3, 2023 17:13
iritkatriel added 3 commits March 6, 2023 10:03
@iritkatriel
Merge branch 'main' into fetch-restore-objects
44bead6
@iritkatriel
Merge branch 'main' into fetch-restore-objects
d592920
@iritkatriel
Merge branch 'main' into fetch-restore-objects
e136989
markshannon
markshannon approved these changes Mar 8, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels Mar 8, 2023
@iritkatriel
iritkatriel merged commit 11a2c6c into python:main Mar 8, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Mar 8, 2023
carljm added a commit to carljm/cpython that referenced this pull request Mar 8, 2023
@carljm
Merge branch 'main' into inlinecomp2
06db319 
* main:
  pythongh-102304: Consolidate Direct Usage of _Py_RefTotal (pythongh-102514)
  pythongh-102192: Replace PyErr_Fetch/Restore etc by more efficient alternatives (in Objects/) (python#102218)
  pythongh-102507 Remove invisible pagebreak characters (python#102531)
  pythongh-102515: Remove unused imports in the `Lib/` directory (python#102516)
  Remove or update bitbucket links (pythonGH-101963)
  pythongh-101100: Fix sphinx warnings in `zipapp` and `zipfile` modules (python#102526)
  pythonGH-102397: Fix segfault from race condition in signal handling (python#102399)
  Fix style in argparse.rst (python#101733)
  Post 3.12.0a6
  fix typo in async generator code field name `ag_code` (python#102448)
  Python 3.12.0a6
@iritkatriel
iritkatriel deleted the fetch-restore-objects branch April 3, 2023 17:45
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
@methane methane Awaiting requested review from methane methane is a code owner
@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland

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.

Uh oh!

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

3 participants

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