Skip to content
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.

Sign up for GitHub

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

Jump to bottom

GH-106485: Dematerialize instance dictionaries when possible #106539

Merged
merged 22 commits into from
Aug 9, 2023
Merged

GH-106485: Dematerialize instance dictionaries when possible #106539

Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
a4e456f
"Un-materialize" __dict__s if possible
brandtbucher Jun 22, 2023
716cc5a
Add stats
brandtbucher Jun 29, 2023
b9ec16f
Catch up with main
brandtbucher Jun 29, 2023
c5f2067
Add comment
brandtbucher Jun 29, 2023
0ab8274
Catch up with main
brandtbucher Jul 6, 2023
c3d076b
Catch up with main
brandtbucher Jul 7, 2023
2b20a5b
Un-materialize in LOAD_ATTR_INSTANCE_VALUE
brandtbucher Jul 7, 2023
ebcad24
Catch up with main (sorta)
brandtbucher Jul 7, 2023
5d76456
Fix test_opcache
brandtbucher Jul 7, 2023
d00eefe
Clean up the diff
brandtbucher Jul 7, 2023
2c4f262
Catch up with main
brandtbucher Jul 10, 2023
912e12e
Catch up with main
brandtbucher Jul 14, 2023
222469a
"Dematerialize" in other places too
brandtbucher Jul 15, 2023
94dd38f
Catch up with main
brandtbucher Jul 15, 2023
fe19772
Fix whitespace
brandtbucher Jul 16, 2023
7f4fd05
fixup
brandtbucher Jul 16, 2023
25202d9
blurb add
brandtbucher Jul 16, 2023
d6d2045
Catch up with main
brandtbucher Aug 3, 2023
125a15b
Add missing stat inc
brandtbucher Aug 5, 2023
b2495de
Catch up with main
brandtbucher Aug 5, 2023
d42d2e6
Add another missing increment
brandtbucher Aug 5, 2023
a4e8fbf
Catch up with main
brandtbucher Aug 9, 2023
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
4 changes: 4 additions & 0 deletions Include/internal/pycore_dict.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ extern "C" {
#endif

#include "pycore_dict_state.h"
#include "pycore_object.h"
#include "pycore_runtime.h" // _PyRuntime

// Unsafe flavor of PyDict_GetItemWithError(): no error checking
Expand Down Expand Up @@ -62,6 +63,8 @@ extern uint32_t _PyDictKeys_GetVersionForCurrentState(

extern size_t _PyDict_KeysSize(PyDictKeysObject *keys);

extern void _PyDictKeys_DecRef(PyDictKeysObject *keys);

/* _Py_dict_lookup() returns index of entry which can be used like DK_ENTRIES(dk)[index].
* -1 when no entry found, -3 when compare raises error.
*/
Expand Down Expand Up @@ -196,6 +199,7 @@ _PyDict_NotifyEvent(PyInterpreterState *interp,
}

extern PyObject *_PyObject_MakeDictFromInstanceAttributes(PyObject *obj, PyDictValues *values);
extern int _PyObject_MakeInstanceAttributesFromDict(PyObject *obj, PyDictOrValues *dorv);
extern PyObject *_PyDict_FromItems(
PyObject *const *keys, Py_ssize_t keys_offset,
PyObject *const *values, Py_ssize_t values_offset,
Expand Down
1 change: 1 addition & 0 deletions Include/pystats.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ typedef struct _object_stats {
uint64_t dict_materialized_new_key;
uint64_t dict_materialized_too_big;
uint64_t dict_materialized_str_subclass;
uint64_t dict_dematerialized;
uint64_t type_cache_hits;
uint64_t type_cache_misses;
uint64_t type_cache_dunder_hits;
Expand Down
8 changes: 6 additions & 2 deletions Lib/test/test_opcache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -865,8 +865,10 @@ class C:
items = []
for _ in range(self.ITEMS):
item = C()
item.__dict__
item.a = None
# Resize into a combined unicode dict:
for i in range(29):
setattr(item, f"_{i}", None)
items.append(item)
return items

Expand Down Expand Up @@ -932,7 +934,9 @@ class C:
items = []
for _ in range(self.ITEMS):
item = C()
item.__dict__
# Resize into a combined unicode dict:
for i in range(29):
setattr(item, f"_{i}", None)
items.append(item)
return items

Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2023-07-16-07-55-19.gh-issue-106485.wPb1bH.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Reduce the number of materialized instances dictionaries by dematerializing
them when possible.
33 changes: 33 additions & 0 deletions Objects/dictobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5464,6 +5464,35 @@ _PyObject_MakeDictFromInstanceAttributes(PyObject *obj, PyDictValues *values)
return make_dict_from_instance_attributes(interp, keys, values);
}

// Return 1 if the dict was dematerialized, 0 otherwise.
int
_PyObject_MakeInstanceAttributesFromDict(PyObject *obj, PyDictOrValues *dorv)
{
assert(_PyObject_DictOrValuesPointer(obj) == dorv);
assert(!_PyDictOrValues_IsValues(*dorv));
PyDictObject *dict = (PyDictObject *)_PyDictOrValues_GetDict(*dorv);
if (dict == NULL) {
return 0;
}
// It's likely that this dict still shares its keys (if it was materialized
// on request and not heavily modified):
assert(PyDict_CheckExact(dict));
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should allow dict subclasses?

Copy link
Member

Choose a reason for hiding this comment

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

It can't

assert(_PyType_HasFeature(Py_TYPE(obj), Py_TPFLAGS_HEAPTYPE));
if (dict->ma_keys != CACHED_KEYS(Py_TYPE(obj)) || Py_REFCNT(dict) != 1) {
return 0;
}
assert(dict->ma_values);
// We have an opportunity to do something *really* cool: dematerialize it!
_PyDictKeys_DecRef(dict->ma_keys);
_PyDictOrValues_SetValues(dorv, dict->ma_values);
OBJECT_STAT_INC(dict_dematerialized);
// Don't try this at home, kids:
dict->ma_keys = NULL;
dict->ma_values = NULL;
Py_DECREF(dict);
return 1;
}

int
_PyObject_StoreInstanceAttribute(PyObject *obj, PyDictValues *values,
PyObject *name, PyObject *value)
Expand Down Expand Up @@ -5688,6 +5717,7 @@ PyObject_GenericGetDict(PyObject *obj, void *context)
dict = _PyDictOrValues_GetDict(*dorv_ptr);
if (dict == NULL) {
dictkeys_incref(CACHED_KEYS(tp));
OBJECT_STAT_INC(dict_materialized_on_request);
dict = new_dict_with_shared_keys(interp, CACHED_KEYS(tp));
dorv_ptr->dict = dict;
}
Expand Down Expand Up @@ -5731,6 +5761,9 @@ _PyObjectDict_SetItem(PyTypeObject *tp, PyObject **dictptr,
dict = *dictptr;
if (dict == NULL) {
dictkeys_incref(cached);
if (_PyType_HasFeature(tp, Py_TPFLAGS_MANAGED_DICT)) {
OBJECT_STAT_INC(dict_materialized_on_request);
}
Comment on lines +5764 to +5766
Copy link
Member Author

@brandtbucher brandtbucher Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@markshannon BTW, this was the missing STAT_INC. Anything that sets a custom tp_new and has Py_TPFLAGS_MANAGED_DICT doesn't get its inline values initialized, and has a NULL dict. We handle it fine, but we may want to consider treating a NULL dict as "has values that need initializing" rather than "has a dict that needs initializing".

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out, this is a major source of dict materializations in the benchmarks (about ~4 million dicts that we weren't counting before). It looks like we're creating all of these just to dematerialize them almost immediately.

dict = new_dict_with_shared_keys(interp, cached);
if (dict == NULL)
return -1;
Expand Down
3 changes: 0 additions & 3 deletions Objects/typeobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4965,9 +4965,6 @@ type_setattro(PyTypeObject *type, PyObject *name, PyObject *value)
return res;
}

extern void
_PyDictKeys_DecRef(PyDictKeysObject *keys);


static void
type_dealloc_common(PyTypeObject *type)
Expand Down
18 changes: 12 additions & 6 deletions Python/bytecodes.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1817,8 +1817,10 @@ dummy_func(
op(_CHECK_MANAGED_OBJECT_HAS_VALUES, (owner -- owner)) {
assert(Py_TYPE(owner)->tp_dictoffset < 0);
assert(Py_TYPE(owner)->tp_flags & Py_TPFLAGS_MANAGED_DICT);
PyDictOrValues dorv = *_PyObject_DictOrValuesPointer(owner);
DEOPT_IF(!_PyDictOrValues_IsValues(dorv), LOAD_ATTR);
PyDictOrValues *dorv = _PyObject_DictOrValuesPointer(owner);
Copy link
Member

Choose a reason for hiding this comment

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

This is turning:

  test_bit = owner[-4] & 1
  jump if test_bit -> slow_path

into

  test_bit = owner[-4] & 1
  jump if not test_bit -> following
  spill-registers
  call _PyObject_MakeInstanceAttributesFromDict
  restore-registers
  jump if return-register ->following
  jump -> slow_path
following:

or, which is even worse:

  test_bit = owner[-4] & 1
  spill-registers
  jump if not test_bit -> following
  call _PyObject_MakeInstanceAttributesFromDict
  jump if return-register ->following
  jump -> slow_path
following:
  restore-registers

I think this will need to be

DEOPT_IF(!_PyDictOrValues_IsValues(*dorv), LOAD_ATTR_DEMATERIALIZE)

To keep the slow-path from messing up the code.

Copy link
Member Author

@brandtbucher brandtbucher Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This really complicates the code (and probably needs ugly special-casing in the resulting uop trace), since LOAD_ATTR_DEMATERIALIZE would need to jump back into (or repeat) the instruction we happen to be executing after a successful dematerialization.

Can we leave that to another PR, if we determine that the compiler indeed hasn't just moved this cold branch with the register spills out-of-line (like it should, under PGO)?

  test_bit = owner[-4] & 1
  jump if test_bit -> cold
following:
  ...
cold:
  spill-registers
  call _PyObject_MakeInstanceAttributesFromDict
  restore-registers
  jump if return-register ->following
  jump -> slow_path

DEOPT_IF(!_PyDictOrValues_IsValues(*dorv) &&
!_PyObject_MakeInstanceAttributesFromDict(owner, dorv),
LOAD_ATTR);
}

op(_LOAD_ATTR_INSTANCE_VALUE, (index/1, owner -- res2 if (oparg & 1), res)) {
Expand Down Expand Up @@ -2719,8 +2721,10 @@ dummy_func(
assert(type_version != 0);
DEOPT_IF(self_cls->tp_version_tag != type_version, LOAD_ATTR);
assert(self_cls->tp_flags & Py_TPFLAGS_MANAGED_DICT);
PyDictOrValues dorv = *_PyObject_DictOrValuesPointer(self);
DEOPT_IF(!_PyDictOrValues_IsValues(dorv), LOAD_ATTR);
PyDictOrValues *dorv = _PyObject_DictOrValuesPointer(self);
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as for LOAD_ATTR_INSTANCE_VALUES

DEOPT_IF(!_PyDictOrValues_IsValues(*dorv) &&
!_PyObject_MakeInstanceAttributesFromDict(self, dorv),
LOAD_ATTR);
PyHeapTypeObject *self_heap_type = (PyHeapTypeObject *)self_cls;
DEOPT_IF(self_heap_type->ht_cached_keys->dk_version !=
keys_version, LOAD_ATTR);
Expand Down Expand Up @@ -2749,8 +2753,10 @@ dummy_func(
assert(type_version != 0);
DEOPT_IF(self_cls->tp_version_tag != type_version, LOAD_ATTR);
assert(self_cls->tp_flags & Py_TPFLAGS_MANAGED_DICT);
PyDictOrValues dorv = *_PyObject_DictOrValuesPointer(self);
DEOPT_IF(!_PyDictOrValues_IsValues(dorv), LOAD_ATTR);
PyDictOrValues *dorv = _PyObject_DictOrValuesPointer(self);
Copy link
Member

Choose a reason for hiding this comment

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

ditto

DEOPT_IF(!_PyDictOrValues_IsValues(*dorv) &&
!_PyObject_MakeInstanceAttributesFromDict(self, dorv),
LOAD_ATTR);
PyHeapTypeObject *self_heap_type = (PyHeapTypeObject *)self_cls;
DEOPT_IF(self_heap_type->ht_cached_keys->dk_version !=
keys_version, LOAD_ATTR);
Expand Down
6 changes: 4 additions & 2 deletions Python/executor_cases.c.h
View file
Open in desktop

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

18 changes: 12 additions & 6 deletions Python/generated_cases.c.h
View file
Open in desktop

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

19 changes: 14 additions & 5 deletions Python/specialize.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ print_object_stats(FILE *out, ObjectStats *stats)
fprintf(out, "Object materialize dict (new key): %" PRIu64 "\n", stats->dict_materialized_new_key);
fprintf(out, "Object materialize dict (too big): %" PRIu64 "\n", stats->dict_materialized_too_big);
fprintf(out, "Object materialize dict (str subclass): %" PRIu64 "\n", stats->dict_materialized_str_subclass);
fprintf(out, "Object dematerialize dict: %" PRIu64 "\n", stats->dict_dematerialized);
fprintf(out, "Object method cache hits: %" PRIu64 "\n", stats->type_cache_hits);
fprintf(out, "Object method cache misses: %" PRIu64 "\n", stats->type_cache_misses);
fprintf(out, "Object method cache collisions: %" PRIu64 "\n", stats->type_cache_collisions);
Expand Down Expand Up @@ -686,8 +687,10 @@ specialize_dict_access(
return 0;
}
_PyAttrCache *cache = (_PyAttrCache *)(instr + 1);
PyDictOrValues dorv = *_PyObject_DictOrValuesPointer(owner);
if (_PyDictOrValues_IsValues(dorv)) {
PyDictOrValues *dorv = _PyObject_DictOrValuesPointer(owner);
if (_PyDictOrValues_IsValues(*dorv) ||
_PyObject_MakeInstanceAttributesFromDict(owner, dorv))
{
// Virtual dictionary
PyDictKeysObject *keys = ((PyHeapTypeObject *)type)->ht_cached_keys;
assert(PyUnicode_CheckExact(name));
Expand All @@ -705,12 +708,16 @@ specialize_dict_access(
instr->op.code = values_op;
}
else {
PyDictObject *dict = (PyDictObject *)_PyDictOrValues_GetDict(dorv);
PyDictObject *dict = (PyDictObject *)_PyDictOrValues_GetDict(*dorv);
if (dict == NULL || !PyDict_CheckExact(dict)) {
SPECIALIZATION_FAIL(base_op, SPEC_FAIL_NO_DICT);
return 0;
}
// We found an instance with a __dict__.
if (dict->ma_values) {
Copy link
Member

Choose a reason for hiding this comment

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

This is quite a complex condition. The goto makes it even harder to follow.
Could you compute a virtual_dict flag, then branch on that?

SPECIALIZATION_FAIL(base_op, SPEC_FAIL_ATTR_NON_STRING_OR_SPLIT);
return 0;
}
Py_ssize_t index =
_PyDict_LookupIndex(dict, name);
if (index != (uint16_t)index) {
Expand Down Expand Up @@ -1093,9 +1100,11 @@ PyObject *descr, DescriptorClassification kind, bool is_method)
assert(descr != NULL);
assert((is_method && kind == METHOD) || (!is_method && kind == NON_DESCRIPTOR));
if (owner_cls->tp_flags & Py_TPFLAGS_MANAGED_DICT) {
PyDictOrValues dorv = *_PyObject_DictOrValuesPointer(owner);
PyDictOrValues *dorv = _PyObject_DictOrValuesPointer(owner);
PyDictKeysObject *keys = ((PyHeapTypeObject *)owner_cls)->ht_cached_keys;
if (!_PyDictOrValues_IsValues(dorv)) {
if (!_PyDictOrValues_IsValues(*dorv) &&
!_PyObject_MakeInstanceAttributesFromDict(owner, dorv))
{
SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_HAS_MANAGED_DICT);
return 0;
}
Expand Down
Loading