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

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

Issue: Reduce the number of materialized instance __dict__s #106485

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.

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?

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

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?

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

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

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

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

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

Same comment as for LOAD_ATTR_INSTANCE_VALUES

ditto

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. 🙃

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

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

Maybe this should allow dict subclasses?

It can't

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

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

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

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

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

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

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

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

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

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

Uh oh!

GH-106485: Dematerialize instance dictionaries when possible - #106539

GH-106485: Dematerialize instance dictionaries when possible#106539brandtbucher merged 22 commits intopython:mainpython/cpython:mainfrom brandtbucher:un-materialize-altbrandtbucher/cpython:un-materialize-altCopy head branch name to clipboard

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

GH-106485: Dematerialize instance dictionaries when possible#106539
brandtbucher merged 22 commits into
python:mainfrom
brandtbucher:un-materialize-alt

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