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

GH-93143: Don't turn`LOAD_FAST` into`LOAD_FAST_CHECK`#99075

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 6 commits intopython:mainfrombrandtbucher:maybe-bind-fast-local

Nov 8, 2022

Merged

GH-93143: Don't turn`LOAD_FAST` into`LOAD_FAST_CHECK`#99075

brandtbucher merged 6 commits intopython:mainfrombrandtbucher:maybe-bind-fast-local

Nov 8, 2022

Conversation

Copy link

Member

brandtbucher commentedNov 3, 2022•
edited by bedevere-bot
Loading

The compiler's dataflow analysis for fast locals can be invalidated by jumps or deletions while tracing. When this happens, rather than changingco_code (which users assume to be constant) to use the less-efficientLOAD_FAST_CHECK everywhere, just display aRuntimeWarning and set any problematic locals toNone instead.

Issue:Dataflow analysis in the compiler to avoid runtime NULL checks #93143

brandtbucher added6 commits

October 19, 2022 23:45

Just assign None instead of changing co_code

e0f2459

blurb add

6f68d32

Feedback from code review

dd52da5

fixup

da07230

Perform assignments *before* adjusting the stack

555d23e

Address code review

4e597b3

brandtbucher added the interpreter-core(Objects, Python, Grammar, and Parser dirs) label

Nov 3, 2022

brandtbucher requested a review fromsweeneyde

November 3, 2022 20:04

brandtbucher self-assigned this

Nov 3, 2022

bedevere-bot mentioned this pull request

Nov 3, 2022

Dataflow analysis in the compiler to avoid runtime NULL checks#93143

Closed

bedevere-bot added the awaiting core review label

Nov 3, 2022

brandtbucher marked this pull request as ready for review

November 3, 2022 20:04

brandtbucher requested a review frommarkshannon as acode owner

November 3, 2022 20:04

Copy link

Member

sweeneyde commentedNov 3, 2022

I can look closer this evening, but mutating co_code is not ideal, so I think this is probably a good idea.

sweeneyde reviewed

Nov 4, 2022

View reviewed changes

Lib/test/test_peepholer.py OutdatedShow resolvedHide resolved

Objects/frameobject.c OutdatedShow resolvedHide resolved

Copy link

Member

markshannon commentedNov 4, 2022

There are two cases when we need to set locals toNone.

Deletion of locals
Jumping to arbitrary code

In the first case we can set the local toNone and issue the warning cheaply and probably correctly. That's fine.

However when jumping, trying to decide which locals need to be set toNone is going to be complex, expensive and error prone.
So I think we should go with@sweeneyde suggestion of setting all the undefined local variables toNone.
Either don't issue any warning (as it might be confusing), or maybe issue a generic warning only if any locals were set toNone.

sweeneyde mentioned this pull request

Nov 4, 2022

ConvertDELETE_FAST toLOAD_FAST_CHECK POP_TOP DELETE_FAST as a size optimizationfaster-cpython/ideas#490

Open

Copy link

MemberAuthor

brandtbucher commentedNov 4, 2022

I've updated the code to skip scanning the bytecode forLOAD_FAST. Any unbound locals will be assignedNone on jumps, and (if so) a singleRuntimeWarning will be issued for all of them.

markshannon requested changes

Nov 7, 2022

View reviewed changes

Copy link

Member

markshannon left a comment

Choose a reason for hiding this comment

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

👍
This feels safer and more robust than what we have now.

Lib/test/test_peepholer.pyShow resolvedHide resolved

Misc/NEWS.d/next/Core and Builtins/2022-10-19-23-54-43.gh-issue-93143.1wCYub.rst Outdated

		@@ -0,0 +1,4 @@
		Rather than changing :attr:`~types.CodeType.co_code`, the interpreter will
		now display a :exc:`RuntimeWarning` and assign :const:`None` to any fast
		locals that are incorrectly left unbound after jumps or :keyword:`del`