Repository navigation
bpo-46823: Implement LOAD_FAST__LOAD_ATTR_INSTANCE_VALUE #31484
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
1e2ce9c
dd3e143
b6e0b7b
9f0b4a4
ec5b712
273926c
5da9b2a
6450e4b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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(); | ||
|
|
@@ -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); | ||
|
|
@@ -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: | ||
| { | ||
| // 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. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| STAT_INC(LOAD_FAST__LOAD_ATTR_INSTANCE_VALUE, miss); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| 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(); | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
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_VALUEget de-optimized, the precedingLOAD_FAST__LOAD_ATTR_INSTANCE_VALUEis still valid, just less likely to hit.Given how unlikely a branch to the
LOAD_ATTRis, don't worry about this case.There was a problem hiding this comment.
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, thencache0->indexshould be correct: you can't turn something with a class that has a _PyObject_ValuesPointer into a class with__slots__without modifyingtp_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 acache1->tp_versionand acache0->indexinto 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 samecache1->tp_versionbut acache0->indexindexing 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_VALUEcache from one instruction's perspective but aLOAD_ATTR_ADAPTIVEcache from a different perspective. Is that worth worrying about?There was a problem hiding this comment.
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_VALUEget deoptimised beforeLOAD_FAST__LOAD_ATTR_INSTANCE_VALUE.LOAD_FAST__LOAD_ATTR_INSTANCE_VALUE_misscan be simplified by lettingLOAD_ATTR_INSTANCE_VALUE_missdo the work.Something like: