NotificationsYou must be signed in to change notification settings
Fork32.1k
Star67.3k

GH-119866: Don't clear`frame->stackpointer` on release builds#131750

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.

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

Jump to bottom

Merged

brandtbucher merged 2 commits intopython:mainfrombrandtbucher:null-stack-pointer

Mar 26, 2025

Merged

GH-119866: Don't clear`frame->stackpointer` on release builds#131750

brandtbucher merged 2 commits intopython:mainfrombrandtbucher:null-stack-pointer

Mar 26, 2025

Conversation

Copy link

Member

brandtbucher commentedMar 26, 2025•
edited by bedevere-appbot
Loading

This is only useful for asserts to make sure that we're spilling and restoring the stack pointer in all the right places. Removing these frequent stores makes the interpreter0.7% faster and the JIT0.4% faster.

Issue:Spill the stack pointer across calls in the interpreter. #119866

Don't set the stack pointer to NULL on release builds

7b432e3

brandtbucher added performance

Performance or resource usage

skip news interpreter-core(Objects, Python, Grammar, and Parser dirs) labels

Mar 26, 2025

brandtbucher requested a review frommarkshannon

March 26, 2025 00:44

brandtbucher self-assigned this

Mar 26, 2025

bedevere-appbot added the awaiting core review label

Mar 26, 2025

bedevere-appbot mentioned this pull request

Mar 26, 2025

Spill the stack pointer across calls in the interpreter.#119866

Closed

chris-eibl reviewed

Mar 26, 2025

View reviewed changes

Include/internal/pycore_interpframe.h

		@@ -171,7 +171,9 @@ _PyFrame_GetStackPointer(_PyInterpreterFrame *frame)
		{
		assert(frame->stackpointer != NULL);
		_PyStackRef *sp = frame->stackpointer;
		#ifndef NDEBUG

Copy link

Member

chris-eiblMar 26, 2025•
edited
Loading

Choose a reason for hiding this comment

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

Maybe then update the comment

Havingstackpointer== NULLensuresthatinvalidvaluesarenotvisibletothecycleGC.

Seems that this was done for the cycle GC - if this is no longer needed, just remove the comment?

Out of curiousity: is it safe in release builds to let the cycle GC see invalid values?

Copy link

MemberAuthor

brandtbucherMar 26, 2025

Choose a reason for hiding this comment

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

We want to have "deferred" references on the stack, which means that the GC must be able to seeall stack values, andonly valid stack values.

Setting the stack pointer toNULL is only useful for our asserts, which are sprinkled throughout to make sure that we're spilling and restoring in the correct places. Basically, the GC should never see an invalid stack pointer. On debug builds, we make this easier to detect by setting invalid stack pointers toNULL and checking for this.

Either way, I'll update the comment.