Skip to content

Bad specialization of STORE_ATTR_INSTANCE_VALUE with obj.__dict__ #125610

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

Closed
colesbury opened this issue Oct 16, 2024 · 0 comments
Closed

Bad specialization of STORE_ATTR_INSTANCE_VALUE with obj.__dict__ #125610

colesbury opened this issue Oct 16, 2024 · 0 comments
Labels
3.13 bugs and security fixes 3.14 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage type-bug An unexpected behavior, bug, or error

Comments

@colesbury
Copy link
Contributor

colesbury commented Oct 16, 2024

Consider:

class MyObject: pass

def func():
    o = MyObject()
    o.__dict__
    for _ in range(100):
        o.foo = "bar"
        o.baz = "qux"

for _ in range(100):
    func()
    opcode[STORE_ATTR_INSTANCE_VALUE].specialization.miss : 20382
    opcode[STORE_ATTR_INSTANCE_VALUE].execution_count : 21167

The STORE_ATTR_INSTANCE_VALUE has a guard _GUARD_DORV_NO_DICT that ensures that the object does not have a managed dictionary:

EXIT_IF(_PyObject_GetManagedDict(owner_o));

However, the specializer for STORE_ATTR_INSTANCE_VALUE does not take that into account. It only checks that the inline values are valid:

cpython/Python/specialize.c

Lines 867 to 886 in 760872e

if (type->tp_flags & Py_TPFLAGS_INLINE_VALUES && _PyObject_InlineValues(owner)->valid) {
PyDictKeysObject *keys = ((PyHeapTypeObject *)type)->ht_cached_keys;
assert(PyUnicode_CheckExact(name));
Py_ssize_t index = _PyDictKeys_StringLookup(keys, name);
assert (index != DKIX_ERROR);
if (index == DKIX_EMPTY) {
SPECIALIZATION_FAIL(base_op, SPEC_FAIL_ATTR_NOT_IN_KEYS);
return 0;
}
assert(index >= 0);
char *value_addr = (char *)&_PyObject_InlineValues(owner)->values[index];
Py_ssize_t offset = value_addr - (char *)owner;
if (offset != (uint16_t)offset) {
SPECIALIZATION_FAIL(base_op, SPEC_FAIL_OUT_OF_RANGE);
return 0;
}
write_u32(cache->version, type->tp_version_tag);
cache->index = (uint16_t)offset;
instr->op.code = values_op;
}

I'm not sure if we should change the guard or change specialize.c

Linked PRs

@colesbury colesbury added performance Performance or resource usage 3.13 bugs and security fixes 3.14 bugs and security fixes labels Oct 16, 2024
colesbury added a commit to colesbury/cpython that referenced this issue Oct 16, 2024
The `STORE_ATTR_INSTANCE_VALUE` opcode doesn't support objects with
non-NULL managed dictionaries, so don't specialize to that op in that case.
@picnixz picnixz added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Oct 21, 2024
colesbury added a commit to colesbury/cpython that referenced this issue Dec 4, 2024
colesbury added a commit that referenced this issue Dec 6, 2024
…25612)

The `STORE_ATTR_INSTANCE_VALUE` opcode doesn't support objects with
non-NULL managed dictionaries, so don't specialize to that op in that case.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Dec 6, 2024
…pythonGH-125612)

The `STORE_ATTR_INSTANCE_VALUE` opcode doesn't support objects with
non-NULL managed dictionaries, so don't specialize to that op in that case.
(cherry picked from commit a353455)

Co-authored-by: Sam Gross <[email protected]>
colesbury added a commit that referenced this issue Dec 6, 2024
GH-125612) (GH-127698)

The `STORE_ATTR_INSTANCE_VALUE` opcode doesn't support objects with
non-NULL managed dictionaries, so don't specialize to that op in that case.
(cherry picked from commit a353455)

Co-authored-by: Sam Gross <[email protected]>
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this issue Jan 8, 2025
…pythonGH-125612)

The `STORE_ATTR_INSTANCE_VALUE` opcode doesn't support objects with
non-NULL managed dictionaries, so don't specialize to that op in that case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.13 bugs and security fixes 3.14 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

2 participants