Movatterモバイル変換

Skip to content

python/cpythonPublic

NotificationsYou must be signed in to change notification settings
Fork34k
Star71.3k

gh-143183: Link trace to side exits, rather than stop#143268

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Fidget-Spinner merged 5 commits intopython:mainfromFidget-Spinner:fix-jit-side-exit

Dec 29, 2025

Merged

gh-143183: Link trace to side exits, rather than stop#143268

Fidget-Spinner merged 5 commits intopython:mainfromFidget-Spinner:fix-jit-side-exit

Dec 29, 2025

Conversation

@Fidget-Spinner

Copy link

Member

Fidget-Spinner commentedDec 29, 2025•
edited by bedevere-appbot
Loading

#143187 caused a pretty serious regression in JIT performance, mainly because we stop the traceprior to the ENTER_EXECUTOR. This PR restores the old behavior, stopping the traceat the ENTER_EXECUTOR, allowing it to link up to the executor.

The perf regression can be clearly seen here (going up means things got slower):

Issue:JIT: Assertion failure instopiteration_error #143183

@Fidget-Spinner

Link trace to side exits, rather than stop

1248cfe

@Fidget-Spinner

Fidget-Spinner requested a review frommarkshannon as acode owner

December 29, 2025 10:50

@bedevere-app

bedevere-appbot added the awaiting core review label

@bedevere-app

bedevere-appbot mentioned this pull request

JIT: Assertion failure instopiteration_error#143183

Closed

@Fidget-Spinner

Fidget-Spinner added the skip news label

@Fidget-Spinner

Copy link

MemberAuthor

Fidget-Spinner commentedDec 29, 2025

Before:

After (notice how more things are linked):

@Fidget-Spinner

simplify control flow

d5c2e03

diegorusso

diegorusso reviewed

View reviewed changes

Copy link

Contributor

diegorusso left a comment

Choose a reason for hiding this comment

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

I see that#143187 add a test. Can we have it here as well?

Fidget-Spinner added2 commits

December 29, 2025 11:39

@Fidget-Spinner

Add test for exit

9ea007d

@Fidget-Spinner

fix comment

a9548b8

@Fidget-Spinner

Copy link

MemberAuthor

Fidget-Spinner commentedDec 29, 2025

I fixed the test in the test suite so that it catches it. I verified that without the change, the test fails. With the change, the test passes.

@Fidget-Spinner

Copy link

MemberAuthor

Fidget-Spinner commentedDec 29, 2025

Also verified that:

At commit713684d (before my previous PR), this test passes.
At commitdaa9aa4 (the first commit with my PR), this test fails.
At commita9548b8 (current PR commit), this test passes.

So this does indeed restore the old behavior!

@Fidget-Spinner

Copy link

MemberAuthor

Fidget-Spinner commentedDec 29, 2025

For the following code:

TIER2_THRESHOLD = 4001def f():    for x in range(TIER2_THRESHOLD + 3):        for y in range(TIER2_THRESHOLD + 3):            z = x + yimport sysf()sys._dump_tracelets("hello.gvz")

Current main (buggy):

This PR (notice how the traces now link up):

diegorusso

diegorusso reviewed

View reviewed changes

Lib/test/test_capi/test_opt.py OutdatedShow resolvedHide resolved

@Fidget-Spinner

Address review

8834324

diegorusso

diegorusso approved these changes

View reviewed changes

Copy link

Contributor

diegorusso left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the regression and adding the test for it!

@bedevere-app

bedevere-appbot added awaiting merge and removed awaiting core review labels

@Fidget-Spinner

Fidget-Spinner merged commit6cb245d intopython:main

66 checks passed

@bedevere-app

bedevere-appbot removed the awaiting merge label

@Fidget-Spinner

Fidget-Spinner deleted the fix-jit-side-exit branch

December 29, 2025 15:10

tomasr8

tomasr8 approved these changes

View reviewed changes

Copy link

Member

tomasr8 left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Damn you're too fast.. was about to hit approve when you merged it 😆

Python/optimizer.c

		DPRINTF(2, "Unsupported: oparg too large\n");
		unsupported:
		{
		// Rewind to previous instruction and replace with _EXIT_TRACE.

Copy link

Member

tomasr8Dec 29, 2025

Choose a reason for hiding this comment

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

In the other PR, I meant to update this comment but GH shows the suggestions in a weird a way for unchanges lines.. The point is the comment mentions_EXIT_TRACE but it should be_DEOPT now :)

// Rewind to previous instruction and replace with _EXIT_TRACE.

Copy link

MemberAuthor

Fidget-SpinnerDec 29, 2025

Choose a reason for hiding this comment

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

Oh yeah. Good point!

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Labels

3 participants

@Fidget-Spinner

@diegorusso

@tomasr8

[8]ページ先頭

©2009-2026 Movatter.jp