Skip to content

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