Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Implement LOAD_FAST__LOAD_ATTR_INSTANCE_VALUE
  • Loading branch information
sweeneyde committed Feb 22, 2022
commit 1e2ce9cef628acb8e83d98a182ffd6c1e9cf3eaa
1 change: 1 addition & 0 deletions Include/opcode.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Lib/opcode.py
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,7 @@ def jabs_op(name, op):
"LOAD_FAST__LOAD_CONST",
"LOAD_CONST__LOAD_FAST",
"STORE_FAST__STORE_FAST",
"LOAD_FAST__LOAD_ATTR_INSTANCE_VALUE",
]
_specialization_stats = [
"success",
Expand Down
83 changes: 80 additions & 3 deletions Python/ceval.c
Original file line number Diff line number Diff line change
Expand Up @@ -1770,6 +1770,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr
}

TARGET(LOAD_FAST) {
PREDICTED(LOAD_FAST);
PyObject *value = GETLOCAL(oparg);
if (value == NULL) {
goto unbound_local_error;
Expand Down Expand Up @@ -3446,6 +3447,35 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr
}
}

TARGET(LOAD_FAST__LOAD_ATTR_INSTANCE_VALUE) {
assert(cframe.use_tracing == 0);
PyObject *owner = GETLOCAL(oparg); // borrowed
if (owner == NULL) {
goto unbound_local_error;
}
// GET_CACHE(), but for the following opcode
assert(_Py_OPCODE(*next_instr) == LOAD_ATTR_INSTANCE_VALUE);
SpecializedCacheEntry *caches = _GetSpecializedCacheEntryForInstruction(
first_instr, INSTR_OFFSET() + 1, _Py_OPARG(*next_instr));
_PyAdaptiveEntry *cache0 = &caches[0].adaptive;
_PyAttrCache *cache1 = &caches[-1].attr;
assert(cache1->tp_version != 0);
PyTypeObject *tp = Py_TYPE(owner);
// Note: these DEOPT_IFs jump back to LOAD_FAST.
DEOPT_IF(tp->tp_version_tag != cache1->tp_version,
LOAD_FAST__LOAD_ATTR_INSTANCE_VALUE);
assert(tp->tp_dictoffset < 0);
assert(tp->tp_flags & Py_TPFLAGS_MANAGED_DICT);
PyDictValues *values = *_PyObject_ValuesPointer(owner);
DEOPT_IF(values == NULL, LOAD_FAST__LOAD_ATTR_INSTANCE_VALUE);
PyObject *res = values->values[cache0->index];
DEOPT_IF(res == NULL, LOAD_FAST__LOAD_ATTR_INSTANCE_VALUE);
STAT_INC(LOAD_ATTR, hit);
PUSH(Py_NewRef(res));
next_instr++;
NOTRACE_DISPATCH();
}

TARGET(LOAD_ATTR_INSTANCE_VALUE) {
assert(cframe.use_tracing == 0);
PyObject *owner = TOP();
Expand All @@ -3455,13 +3485,13 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr
_PyAdaptiveEntry *cache0 = &caches[0].adaptive;
_PyAttrCache *cache1 = &caches[-1].attr;
assert(cache1->tp_version != 0);
DEOPT_IF(tp->tp_version_tag != cache1->tp_version, LOAD_ATTR);
DEOPT_IF(tp->tp_version_tag != cache1->tp_version, LOAD_ATTR_INSTANCE_VALUE);
assert(tp->tp_dictoffset < 0);
assert(tp->tp_flags & Py_TPFLAGS_MANAGED_DICT);
PyDictValues *values = *_PyObject_ValuesPointer(owner);
DEOPT_IF(values == NULL, LOAD_ATTR);
DEOPT_IF(values == NULL, LOAD_ATTR_INSTANCE_VALUE);
res = values->values[cache0->index];
DEOPT_IF(res == NULL, LOAD_ATTR);
DEOPT_IF(res == NULL, LOAD_ATTR_INSTANCE_VALUE);
STAT_INC(LOAD_ATTR, hit);
Py_INCREF(res);
SET_TOP(res);
Expand Down Expand Up @@ -5430,6 +5460,53 @@ MISS_WITH_CACHE(BINARY_SUBSCR)
MISS_WITH_CACHE(UNPACK_SEQUENCE)
MISS_WITH_OPARG_COUNTER(STORE_SUBSCR)

LOAD_ATTR_INSTANCE_VALUE_miss:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This shouldn't need special casing. If the LOAD_ATTR_INSTANCE_VALUE get de-optimized, the preceding LOAD_FAST__LOAD_ATTR_INSTANCE_VALUE is still valid, just less likely to hit.
Given how unlikely a branch to the LOAD_ATTR is, don't worry about this case.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Since the combined instruction borrows the cache from the following instruction, I was worried about the combined instruction misinterpreting some of the cache data, so that's why I was attempting to keep them in sync.

But it seems that the instructions in the LOAD_ATTR family all interpret the cache entries in the same way, so for now anyway, if there's ever a correct cache1->tp_version, then cache0->index should be correct: you can't turn something with a class that has a _PyObject_ValuesPointer into a class with __slots__ without modifying tp_version_tag. (right?)

This might not be true though if we ever specialize instance.class_attribute. In that scenario, a LOAD_ATTR_INSTANCE_VALUE cache entry could have a cache1->tp_version and a cache0->index into the values array of the instance, but then later get de-optimized and re-optimized to the hypothetical LOAD_ATTR_CLASS_ATTRIBUTE, which could (?) have the same cache1->tp_version but a cache0->index indexing into types's value array.

It feels less fragile to me to keep the adjacent instructions in sync, so that cache is only interpreted in one way at a time -- never as a LOAD_ATTR_INSTANCE_VALUE cache from one instruction's perspective but a LOAD_ATTR_ADAPTIVE cache from a different perspective. Is that worth worrying about?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, it does make sense to special case LOAD_ATTR_INSTANCE_VALUE_miss.
I didn't properly consider the case where LOAD_ATTR_INSTANCE_VALUE get deoptimised before LOAD_FAST__LOAD_ATTR_INSTANCE_VALUE.

LOAD_FAST__LOAD_ATTR_INSTANCE_VALUE_miss can be simplified by letting LOAD_ATTR_INSTANCE_VALUE_miss do the work.

Something like:

LOAD_FAST__LOAD_ATTR_INSTANCE_VALUE_miss:
        {
            // This is special-cased because we have a superinstruction
            // that includes a specialized instruction. 
            // We execute the first instruction, then perform a miss for
            // the second instruction as usual.

            // Do LOAD_FAST
            {
                PyObject *value = GETLOCAL(oparg);
                Py_INCREF(value);
                PUSH(value);
                NEXTOPARG();
                next_instr++;
            }

            // Now we are in the correct state for LOAD_ATTR
            goto LOAD_ATTR_INSTANCE_VALUE_miss;

{
// Special-cased so that if LOAD_ATTR_INSTANCE_VALUE
// gets replaced, then any preceeding
// LOAD_FAST__LOAD_ATTR_INSTANCE_VALUE gets replaced as well
STAT_INC(LOAD_ATTR_INSTANCE_VALUE, miss);
STAT_INC(LOAD_ATTR, miss);
_PyAdaptiveEntry *cache = &GET_CACHE()->adaptive;
cache->counter--;
if (cache->counter == 0) {
next_instr[-1] = _Py_MAKECODEUNIT(LOAD_ATTR_ADAPTIVE, _Py_OPARG(next_instr[-1]));
if (_Py_OPCODE(next_instr[-2]) == LOAD_FAST__LOAD_ATTR_INSTANCE_VALUE) {
next_instr[-2] = _Py_MAKECODEUNIT(LOAD_FAST, _Py_OPARG(next_instr[-2]));
if (_Py_OPCODE(next_instr[-3]) == LOAD_FAST) {
next_instr[-3] = _Py_MAKECODEUNIT(LOAD_FAST__LOAD_FAST, _Py_OPARG(next_instr[-3]));
}
}
STAT_INC(opname, deopt);
cache_backoff(cache);
}
oparg = cache->original_oparg;
JUMP_TO_INSTRUCTION(LOAD_ATTR);
}

LOAD_FAST__LOAD_ATTR_INSTANCE_VALUE_miss:
{
// Special-cased because the standard opcode does not push
// "owner" to the stack, but we need to push it if we DEOPT.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems misleading. We don't need to push the local here because the jump to LOAD_FAST does it.
Maybe:
"Special cased as this is a specialization of LOAD_FAST, but de-optimization occurs during the following LOAD_ATTR instruction.

STAT_INC(LOAD_FAST__LOAD_ATTR_INSTANCE_VALUE, miss);

@markshannon markshannon Feb 23, 2022 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we want to count this as a miss. It is LOAD_ATTR_INSTANCE_VALUE that misses and we don't want to count the miss twice.

STAT_INC(LOAD_ATTR, miss);
SpecializedCacheEntry *caches = _GetSpecializedCacheEntryForInstruction(
first_instr, INSTR_OFFSET() + 1, _Py_OPARG(*next_instr));
_PyAdaptiveEntry *cache0 = &caches[0].adaptive;
cache0->counter--;
if (cache0->counter == 0) {
assert(_Py_OPCODE(next_instr[0]) == LOAD_ATTR_INSTANCE_VALUE);
next_instr[0] = _Py_MAKECODEUNIT(LOAD_ATTR_ADAPTIVE, _Py_OPARG(next_instr[0]));
assert(_Py_OPCODE(next_instr[-1]) == LOAD_FAST__LOAD_ATTR_INSTANCE_VALUE);
next_instr[-1] = _Py_MAKECODEUNIT(LOAD_FAST, _Py_OPARG(next_instr[-1]));
if (_Py_OPCODE(next_instr[-2]) == LOAD_FAST) {
next_instr[-2] = _Py_MAKECODEUNIT(LOAD_FAST__LOAD_FAST, _Py_OPARG(next_instr[-2]));
}
STAT_INC(LOAD_ATTR, deopt);
}
JUMP_TO_INSTRUCTION(LOAD_FAST);
}

binary_subscr_dict_error:
{
PyObject *sub = POP();
Expand Down
2 changes: 1 addition & 1 deletion Python/opcode_targets.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions Python/specialize.c
Original file line number Diff line number Diff line change
Expand Up @@ -889,6 +889,16 @@ _Py_Specialize_LoadAttr(PyObject *owner, _Py_CODEUNIT *instr, PyObject *name, Sp
return -1;
}
if (err) {
if (_Py_OPCODE(instr[0]) == LOAD_ATTR_INSTANCE_VALUE) {
// Note: instr[-1] exists because there's something on the stack,
// and instr[-2] exists because there's at least a RESUME as well.
if (_Py_OPCODE(instr[-1]) == LOAD_FAST) {
instr[-1] = _Py_MAKECODEUNIT(LOAD_FAST__LOAD_ATTR_INSTANCE_VALUE, _Py_OPARG(instr[-1]));
if (_Py_OPCODE(instr[-2]) == LOAD_FAST__LOAD_FAST) {
instr[-2] = _Py_MAKECODEUNIT(LOAD_FAST, _Py_OPARG(instr[-2]));
}
}
}
goto success;
}
fail:
Expand Down