Merging LOAD_METHOD and LOAD_ATTR for better specialization

[Fidget-Spinner]

Currently, a good portion of LOAD_METHOD failures are that the method is an instance attribute.

I've always wondered if we could overwrite LOAD_METHOD with LOAD_ATTR, then let LOAD_ATTR specialize for attribute accesses. The only change we'd need to do is that LOAD_ATTR now needs to push NULL onto the stack, but we're on that path anyways (with LOAD_GLOBAL pushing NULL for calls as well. We also need to ensure that inline_cache_entries(LOAD_METHOD) >= inline_cache_entries(LOAD_ATTR)

My main concern is that this will break certain assumptions by code introspection tools. However, I don't know what's the extent.

Another obvious downside is that we may lose the unbound method optimisation. That's roughly a 20% speedup lost. LOAD_ATTR specialization itself is a 20% gain though so it might balance out. Also I don't expect many unbound methods in such scenarios.

[markshannon]

If I understand correctly, what you are suggesting is similar to what we do with LOAD_GLOBAL.
oparg would become (index of name)<<1 | (push null?) which would potentially make LOAD_ATTR a tad slower, but get rid of a lot of duplication in the specialized forms and allow specialization of LOAD_METHOD for "normal" attributes.
Is that correct?

When you say "the unbound method optimisation" do you mean avoiding the creation of bound methods, or something else?

If you want to try this, then I'd suggest keeping LOAD_METHOD and LOAD_ATTR as "virtual" instructions in the compiler, and replace them with LOAD_ATTR_OR_METHOD in the assembler. That way the changes to the compiler would be minimized.

[Fidget-Spinner]

I'm suggesting using the specializer instead of the compiler. Using your example again,

LOAD_FAST                0 (x)
LOAD_METHOD              0 (m)    # Pushes NULL, type(x).m
CALL

If m is an instance attribute the specializer does this:

LOAD_FAST                0 (x)
LOAD_ATTR                01 (m)    # Pushes NULL, m
CALL

Eventually, the specializer just specializes that as a typical LOAD_ATTR. And we get all the benefits of that.

This transformation isn't one way, if the LOAD_ATTR_ADAPTIVE index<<1 | 1 instruction at specialization time notices that it's commonly dealing with unbound methods, then it can specialize to LOAD_METHOD. Note that LOAD_ATTR index<<1 | 1 only exists as a specialized form of LOAD_METHOD. It cannot appear via other means.

This makes LOAD_METHOD and LOAD_ATTR index<<1 | 1 interchangeable by the specializer. So the state diagram is something like this:

If we don't want it to be two way, inline_cache_entries(LOAD_METHOD) >= inline_cache_entries(LOAD_ATTR). But if we want them to be interchangeable, their inline cache entry counts must be identical.

[Fidget-Spinner]

Huh, seems like setting LSB=1 for LOAD_ATTR isn't feasible because we run out of attribute names 😆 . I just tried and 128 names isn't enough for LOAD_ATTR. Well now I'm stuck.

[markshannon]

If we do the transform in the compiler, we can use up to 2 billion names, not just 128.

It makes everything else simpler as well: just the normal set of one basic form, one adaptive form and N specialised forms.
That way all the existing logic around de-optimization and quickening should work correctly.

[Fidget-Spinner]

Hmm alright, I'll try the approach of merging both into this following what you described here . The results should be quite interesting.

[markshannon]

I'm looking forward to seeing how it works out.

[markshannon]

I like this idea.

To summarize my thoughts:
(I'm going to call the new instruction LOAD_ATTRIBUTE here)
The transformation from LOAD_METHOD and LOAD_ATTR to LOAD_ATTRIBUTE needs to be done in the bytecode compiler.
We also need to assert that oparg & 1 implies that the attribute will be used as a callable, so we can avoid creating bound-methods for x.m(). We can do this in the assembler with the following transforms:

• LOAD_ATTR n -> LOAD_ATTRIBUTE (n<<1)
• LOAD_METHOD n -> LOAD_ATTRIBUTE (n<<1) | 1

The semantics of LOAD_ATTRIBUTE n is the same as LOAD_ATTR n>>1 with the addition of pushing a NULL before the attribute if n & 1

[gvanrossum]

Wait, so this would require extended args if the attribute reference is >= 128? IIUC this is an index in the co_names array (tuple), which is shared with the names of globals (and possibly others). Maybe we could count statically how many code objects have >= 128 names?

[markshannon]

We already do this for LOAD_GLOBAL and that uses the same tuple of names.

[markshannon]

python/cpython#31933

[Fidget-Spinner]

It's more common than I expected. Just clearing pycache and running python -m test test.test_support, there are 56 LOAD_ATTR instances that would need extended args since their args is >= 128.

[gvanrossum]

OT: I've always wanted to separate out the names used by each opcode (or most) into separate tuples. There are some tools that want to know the globals referenced by a function, and currently these end up reporting attributes as extra globals. (To do better you'd have to symbolically interpret the bytecode stream looking for LOAD_GLOBAL-like opcodes, and that's usually too painful to bother.)

[markshannon]

It might be worth taking a look at #396 before doing this.
The cache for LOAD_METHOD is a lot bigger than for LOAD_ATTR, it would be good to shrink it before merging the instructions.

[Fidget-Spinner]

I opened python/cpython#93430 for this.

The cache for LOAD_METHOD is a lot bigger than for LOAD_ATTR, it would be good to shrink it before merging the instructions.

Agreed. We can reduce the cache size after merging too. Unless the bigger cache actually causes an immediate perf regression.