FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-132732: Clear errors in JIT optimizer on error by Fidget-Spinner · Pull Request #136048 · python/cpython · GitHub

Repository navigation

gh-132732: Clear errors in JIT optimizer on error - #136048

Merged
Fidget-Spinner merged 7 commits into
python:mainfrom
Fidget-Spinner:clear-error-on-optimizer-failure
Sep 15, 2025
Merged

Fidget-Spinner merged 7 commits into
python:mainfrom
Fidget-Spinner:clear-error-on-optimizer-failure

Conversation

Fidget-Spinner commented Jun 27, 2025 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

tomasr8 commented Jul 1, 2025

Copy link
Copy Markdown
Member

Asking just to educate myself: what kind of errors are we expecting inside the JIT and is it safe to always clear them?

brandtbucher commented Jul 1, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

In general, we want to shift as much computation as possible to optimization-time instead of runtime. Some classes of errors, like MemoryError, are unavoidable. In these cases, we can just ignore them and trust that they'll either resolve themselves or surface in normal user code soon (it's a bit confusing if we raise an error like this in seemingly "random" places where JIT compilation may kick in).

The other class of errors are ones that are easier to ask forgiveness than permission for. For example, constant-evaluating stuff like a / b, a % b, a >> b, a << b, a[b], and so on, where a and b are known "safe" constants in the JIT. It's way easier to just try the operation and assume it will succeed, and just walk back any errors when it doesn't (in this latter case, the error will just happen at runtime when the operation is evaluated "for real").

Comment thread Python/optimizer_analysis.c Outdated
Comment on lines +562 to +564
if (PyErr_Occurred()) {
PyErr_Clear();
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Shouldn't this be:

Suggested change
if (PyErr_Occurred()) {
PyErr_Clear();
}
assert(PyErr_Occurred());
PyErr_Clear();

...or are there cases where an error hasn't occurred, but we end up here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think the current code goes to error on some cases where we run out of memory too, but no memoryerror is set.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't think so, all uses of goto error in these files seem like they have an error set.

Copy link
Copy Markdown
Member Author

@tomasr8 may I trouble you with a review please?

tomasr8 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Given Brandt's explanation, this looks good to me :) I just left one question/comment

Comment thread Python/optimizer_analysis.c Outdated
Fidget-Spinner merged commit 46f823b into python:main Sep 15, 2025
54 checks passed
Fidget-Spinner deleted the clear-error-on-optimizer-failure branch September 15, 2025 16:24
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants


Back | FazBrowse Home | New Git URL