FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

GH-106485: "Un-materialize" `__dict__`s in `LOAD_ATTR_WITH_HINT` by brandtbucher · Pull Request #106496 · python/cpython · GitHub

/ cpython Public

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

Closed
brandtbucher wants to merge 9 commits into
python:mainfrom
brandtbucher:un-materialize
Closed

brandtbucher wants to merge 9 commits into
python:mainfrom
brandtbucher:un-materialize

Conversation

brandtbucher commented Jul 6, 2023
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

There's a failure path in the specialized bytecode that is often hit by objects that have a materialized __dict__, but probably don't need it anymore.

I'm running the benchmarks and gathering stats to see how promising this approach is.

brandtbucher added performance Performance or resource usage interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Jul 6, 2023
brandtbucher self-assigned this Jul 6, 2023
brandtbucher requested a review from markshannon July 6, 2023 23:10

markshannon commented Jul 7, 2023
edited
Loading

Copy link
Copy Markdown
Member

Copy link
Copy Markdown
Member

There a few scenarios that we should consider. Here are the two I'm concerned about:

  1. One or more objects have their __dict__s accessed, then an attribute accessed. Alternately
  2. Many objects have their __dict__s accessed (e.g. by copy or pickle), then attributes are accessed repeatedly

In case 1, we will repeatedly materialize and dematerialize the __dict__. Hopefully this case will be rare, so the performance impact will be acceptable.

It is case 2 that matters, IMO. We need to keep the relevant LOAD_ATTRs specialized to LOAD_ATTR_INSTANCE_VALUES and at the same time dematerialize the __dict__s when we can.

That suggests to me that dematerialization should occur in the specializer and, more importantly, in the deopt path of LOAD_ATTR_INSTANCE_VALUES.

In LOAD_ATTR_INSTANCE_VALUES
we could replace
DEOPT_IF(!_PyDictOrValues_IsValues(dorv), LOAD_ATTR);
with
DEOPT_IF(!_PyDictOrValues_IsValues(dorv), LOAD_ATTR_DEMATERIALIZE)
and eliminate LOAD_ATTR_WITH_HINT altogether.

brandtbucher commented Jul 7, 2023
edited
Loading

Copy link
Copy Markdown
Member Author

According to the stats comparison, the number of __dict__ materializations "on request" increased from 3.7 million to 3.9 million, but the number of dict "un-materializations" is 3.7 million.

So this is incredibly effective, but the results do suggest that some __dict__s are "thrashing" back and forth in the mypy benchmark, which got 17% slower and pulled the (otherwise boring) results down to 0.6% slower.

(Sorry, it looks like there aren't public links for these results.)

brandtbucher commented Jul 7, 2023
edited
Loading

Copy link
Copy Markdown
Member Author

In case 1, we will repeatedly materialize and dematerialize the __dict__. Hopefully this case will be rare, so the performance impact will be acceptable.

See my comment above: the numbers suggest that the mypy benchmark does this, with quite painful results.

It is case 2 that matters, IMO. We need to keep the relevant LOAD_ATTRs specialized to LOAD_ATTR_INSTANCE_VALUES and at the same time dematerialize the __dict__s when we can.

That suggests to me that dematerialization should occur in the specializer and, more importantly, in the deopt path of LOAD_ATTR_INSTANCE_VALUES.

I think this should stay out of the specializer, since that runs infrequently and only sees the first instance of a class at a given location. I'll try the LOAD_ATTR_INSTANCE_VALUE flavor, though.

Copy link
Copy Markdown
Member Author

Something about my merge messed up the diff, I think...

brandtbucher reopened this Jul 7, 2023
brandtbucher changed the title GH-106485: "Un-materialize" __dict__s if possible GH-106485: "Un-materialize" __dict__s in LOAD_ATTR_WITH_HINT Jul 7, 2023

Copy link
Copy Markdown
Member Author

#106539 is an alternative to this, using LOAD_ATTR_INSTANCE_VALUE.

Copy link
Copy Markdown
Member Author

Closing in favor of #106539.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants


Back | FazBrowse Home | New Git URL