Skip to content

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

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
wants to merge 9 commits into from

Conversation

brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Jul 6, 2023

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 brandtbucher added performance Performance or resource usage interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Jul 6, 2023
@brandtbucher brandtbucher self-assigned this Jul 6, 2023
@brandtbucher brandtbucher requested a review from markshannon July 6, 2023 23:10
@markshannon
Copy link
Member

markshannon commented Jul 7, 2023

@markshannon
Copy link
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
Copy link
Member Author

brandtbucher commented Jul 7, 2023

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
Copy link
Member Author

brandtbucher commented Jul 7, 2023

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.

@brandtbucher
Copy link
Member Author

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

@brandtbucher brandtbucher reopened this Jul 7, 2023
@brandtbucher 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
@brandtbucher
Copy link
Member Author

#106539 is an alternative to this, using LOAD_ATTR_INSTANCE_VALUE.

@brandtbucher
Copy link
Member Author

Closing in favor of #106539.

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