GH-106485: Dematerialize instance dictionaries when possible by brandtbucher · Pull Request #106539 · python/cpython

brandtbucher · 2023-07-07T23:20:51Z

Here's an alternative to #106496, using LOAD_ATTR_INSTANCE_VALUE.

Issue: Reduce the number of materialized instance __dict__s #106485

markshannon

I think this does the right thing, and does it the right way.

But I don't know if it does it at the right time, or in the right place.
In other words, are we dematerializing in the right instruction, and in such as way as to not degrade specialization?

I guess the measure of that is the stats.

markshannon · 2023-07-12T10:50:56Z

Python/bytecodes.c

-            DEOPT_IF(!_PyDictOrValues_IsValues(dorv), LOAD_ATTR);
-            res = _PyDictOrValues_GetValues(dorv)->values[index];
+            PyDictOrValues *dorv = _PyObject_DictOrValuesPointer(owner);
+            if (!_PyDictOrValues_IsValues(*dorv)) {


I don't think we want this big chunk of code inline here, it's going to mess up the tier 2 optimizer (it won't help the compiler generate code for the tier 1 interpreter either).

How about DEOPT_IF(!_PyDictOrValues_IsValues(dorv), LOAD_ATTR_INSTANCE_DEMATERIALIZE)
and make another instruction LOAD_ATTR_INSTANCE_DEMATERIALIZE to do the dematerialization?

markshannon · 2023-07-12T10:52:44Z

Python/specialize.c

    if (_PyDictOrValues_IsValues(dorv)) {
        // Virtual dictionary
+    unmaterializing:
+        ;  // Load-bearing semicolon; don't touch!


Maybe // C doesn't allow declarations immediately after a label?

markshannon · 2023-07-12T10:57:00Z

Python/specialize.c

            return 0;
        }
        // We found an instance with a __dict__.
+        if (dict->ma_values) {


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?

markshannon · 2023-07-12T12:51:39Z

The stats still show "has managed dict" as the primary reason for specialization failure.

That suggests to me that we need to dematerialize in LOAD_ATTR_METHOD_WITH_VALUES and LOAD_ATTR_NONDESCRIPTOR_WITH_VALUES as well.

If that is the case we should be able to get the specialization rate for LOAD_ATTR up to 98% (ignoring misses).

brandtbucher · 2023-07-16T20:49:01Z

Just kicked off a stats/benchmarking job for the new approach. Should be done soon.

brandtbucher · 2023-07-16T23:52:57Z

Stats comparison vs base and benchmark comparison vs base. On my phone, so not able to dig into the results right now.

markshannon

The stats are puzzling.
This hugely improves the specialization success rate, but make almost no difference to the number of unspecialized LOAD_ATTRs executed.

markshannon · 2023-07-17T08:08:29Z

Python/bytecodes.c

            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);


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.

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

markshannon · 2023-07-17T08:09:07Z

Python/bytecodes.c

            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);


Same comment as for LOAD_ATTR_INSTANCE_VALUES

markshannon · 2023-07-17T08:09:13Z

Python/bytecodes.c

            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);


brandtbucher · 2023-08-07T19:05:46Z

The stats are puzzling.
This hugely improves the specialization success rate, but make almost no difference to the number of unspecialized LOAD_ATTRs executed.

We're only dematerializing about 5M dicts, and the unspecialized LOAD_ATTR rates drop by about 35M (3%). So, on average, each object that used to have a dict gets about 7 new hits. That seems reasonable.

As a side note, I've learned to mostly ignore the specialization success/failure stats when comparing branches like this, since repeated specialization attempts (and exponential backoff) cause failure rates to grow or shrink disproportionately from the actual number of specialized sites. In this case, the number of successes drops by 236k and the number of failures drop by over a million. But the percentages say we actually have 5% more successes now. It's sort of confusing to derive much meaning from those sorts of numbers. 🙃

brandtbucher · 2023-08-08T22:32:28Z

Confirmed that the weird mypy2 results seem to result from an error in the runner logic for that benchmark.

brandtbucher · 2023-08-09T17:58:29Z

Objects/dictobject.c

+            if (_PyType_HasFeature(tp, Py_TPFLAGS_MANAGED_DICT)) {
+                OBJECT_STAT_INC(dict_materialized_on_request);
+            }


@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".

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.

kumaraditya303 · 2023-08-10T06:53:41Z

Objects/dictobject.c

+    }
+    // It's likely that this dict still shares its keys (if it was materialized
+    // on request and not heavily modified):
+    assert(PyDict_CheckExact(dict));


Maybe this should allow dict subclasses?

brandtbucher added 9 commits June 22, 2023 13:41

"Un-materialize" __dict__s if possible

a4e456f

Add stats

716cc5a

Catch up with main

b9ec16f

Add comment

c5f2067

Catch up with main

0ab8274

Catch up with main

c3d076b

Un-materialize in LOAD_ATTR_INSTANCE_VALUE

2b20a5b

Catch up with main (sorta)

ebcad24

Fix test_opcache

5d76456

brandtbucher added performance Performance or resource usage interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Jul 7, 2023

brandtbucher requested a review from markshannon July 7, 2023 23:20

brandtbucher self-assigned this Jul 7, 2023

bedevere-bot mentioned this pull request Jul 7, 2023

Reduce the number of materialized instance __dict__s #106485

Closed

brandtbucher mentioned this pull request Jul 7, 2023

GH-106485: "Un-materialize" __dict__s in LOAD_ATTR_WITH_HINT #106496

Closed

brandtbucher added 2 commits July 7, 2023 16:35

Clean up the diff

d00eefe

Catch up with main

2c4f262

markshannon reviewed Jul 12, 2023

View reviewed changes

brandtbucher and others added 4 commits July 14, 2023 16:21

Catch up with main

912e12e

"Dematerialize" in other places too

222469a

Catch up with main

94dd38f

Fix whitespace

fe19772

brandtbucher changed the title ~~GH-106485: "Un-materialize" __dict__s in LOAD_ATTR_INSTANCE_VALUE~~ GH-106485: Dematerialize instance dictionaries when possible Jul 16, 2023

brandtbucher added 2 commits July 16, 2023 07:40

fixup

7f4fd05

blurb add

25202d9

brandtbucher marked this pull request as ready for review July 16, 2023 14:56

brandtbucher requested a review from methane as a code owner July 16, 2023 14:56

bedevere-bot added the awaiting core review label Jul 16, 2023

brandtbucher requested a review from markshannon July 16, 2023 20:49

markshannon reviewed Jul 17, 2023

View reviewed changes

brandtbucher added 4 commits August 3, 2023 09:02

Catch up with main

d6d2045

Add missing stat inc

125a15b

Catch up with main

b2495de

Add another missing increment

d42d2e6

brandtbucher requested a review from markshannon August 8, 2023 22:51

brandtbucher commented Aug 9, 2023

View reviewed changes

Catch up with main

a4e8fbf

brandtbucher enabled auto-merge (squash) August 9, 2023 18:30

brandtbucher merged commit 326f0ba into python:main Aug 9, 2023

bedevere-bot removed the awaiting core review label Aug 9, 2023

kumaraditya303 reviewed Aug 10, 2023

View reviewed changes

cdce8p mentioned this pull request Feb 21, 2024

Python 3.12 - ValueError: generator already executing pylint-dev/pylint#9138

Closed

encukou mentioned this pull request Feb 22, 2024

Reference count variations after getting __dict__, getattr, and gc.collect #115822

Open

mdickinson mentioned this pull request May 5, 2024

Test failures under Python 3.13 mdickinson/refcycle#105

Open

Uh oh!

Conversation

brandtbucher commented Jul 7, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markshannon commented Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brandtbucher commented Jul 16, 2023

Uh oh!

brandtbucher commented Jul 16, 2023

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brandtbucher Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brandtbucher commented Aug 7, 2023

Uh oh!

brandtbucher commented Aug 8, 2023

Uh oh!

brandtbucher Aug 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

brandtbucher commented Jul 7, 2023 •

edited by bedevere-bot

Loading

markshannon commented Jul 12, 2023 •

edited

Loading

brandtbucher Aug 7, 2023 •

edited

Loading

brandtbucher Aug 9, 2023 •

edited

Loading