Skip to content

GH-131798: Skip self/NULL checks for some known non-methods - #132278

Closed
brandtbucher wants to merge 3 commits into
python:mainfrom
brandtbucher:non-methods
Closed

GH-131798: Skip self/NULL checks for some known non-methods#132278
brandtbucher wants to merge 3 commits into
python:mainfrom
brandtbucher:non-methods

Conversation

@brandtbucher

@brandtbucherbrandtbucher commented Apr 8, 2025

Copy link
Copy Markdown
Member

CALL_TYPE_1, CALL_STR_1, CALL_TUPLE_1, CALL_LEN, and CALL_ISINSTANCE will never call a method descriptor, which means that we can just assert that self_or_null is NULL instead of checking and "adjusting" the arguments each time.

CALL_BUILTIN_CLASS should never call a method descriptor, but I'm not sure that we can gurantee it. So I've just made this a DEOPT_IF instead.

@brandtbucherbrandtbucher added performance Performance or resource usage interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Apr 8, 2025
@brandtbucherbrandtbucher self-assigned this Apr 8, 2025
@markshannon

Copy link
Copy Markdown
Member

What about this:

defcall_x(a, b):
returna.x(b)
classC: passc=C()
c.x=typecall_x(c, 1); call_x(c, 1)
f=types.MethodType(type, 3)
classD:
x=fd=D()
call_x(d, "string")

@brandtbucher

Copy link
Copy Markdown
MemberAuthor

I'll try, but I'm pretty sure those are fine, since LOAD_ATTR only performs the "unwrapping" optimization for method descriptors (type, str, tuple, len, isisinstance, and types.MethodType aren't).

At any rate, I'll add a test.

@brandtbucher

Copy link
Copy Markdown
MemberAuthor

@markshannon, I went ahead and added a bunch of tests for weird method patterns like that.

@markshannon

Copy link
Copy Markdown
Member

I'm still reluctant to make this change, as it makes the behavior of individual instructions depend on implicit context.
For example, this function:

deff(x, a, b):
returnx(a, b)

currently compiles as

 1 RESUME 0
2 LOAD_FAST_BORROW 0 (x)
PUSH_NULL
LOAD_FAST_BORROW_LOAD_FAST_BORROW 18 (a, b)
CALL 2
RETURN_VALUE

but it could be correctly compiled as:

 1 RESUME 0
2 LOAD_FAST_BORROW 0 (x)
LOAD_FAST_BORROW_LOAD_FAST_BORROW 18 (a, b)
CALL 1
RETURN_VALUE

which would not work with this PR.

@markshannon

Copy link
Copy Markdown
Member

IIRC, we use do just something like that. When compiling m.f(...) if m was defined by an import we would use LOAD_ATTR instead of LOAD_METHOD, so that m.f(a, b) would compile to the then equivalent of CALL 1

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

Labels

awaiting core reviewinterpreter-core(Objects, Python, Grammar, and Parser dirs)performancePerformance or resource usage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@brandtbucher@markshannon