Improve performance of Wrapper components (Chart, Group, Stack) #1494
Conversation
|
@dmmulroy It looks like stacks are not working correctly after this change. You can check out the chromatic diffs here: https://www.chromaticqa.com/build?appId=5b4acf7c54c0490024d5980b&number=409 I'm going to dig into the code, too and see if there's something simple missing. |
| const nestedChildProps = assign({}, c.props, pick(childProps, sharedProps)); | ||
| return React.cloneElement(c, nestedChildProps); | ||
| }); | ||
| const nestedChildren = React.Children.toArray(child.props.children).map((c) => { |
There was a problem hiding this comment.
This is the issue for stacking. The domain won't be correctly calculated if VictoryStack getChildren isn't called when calculating the domain, because the y0 / y1 values wont be set on the children. I'm guessing removing this is also where we're getting most of our perf improvements, though. It's probably worth writing a specialized method for calculating a domain for VictoryStack.
There was a problem hiding this comment.
Thanks for looking into this! I'll start poking around to see what I can figure out 👍
There was a problem hiding this comment.
Hmm... we also have to make sure that VictoryChart and VictoryGroup actually use VictoryStack's getDomain method. Maybe it should be a static on that class the same way it is on components like VictoryBar
There was a problem hiding this comment.
I'm actually looking into potentially having VictoryChart and VictoryGroup properly calculate y0 and y1 in their getCalculatedProps so we can bank on the those values being available/memoized on subsequent/child getCalculateProps calls.
There was a problem hiding this comment.
Oh, I'm using the wrong terms... I mean _y0 and _y1. Values that get added to each data point when we stack values
There was a problem hiding this comment.
I haven't had a ton of time to spend on this since last week, but I did figure out a small win to fix the regression (I think, still need to check chromatic). It does decrease performance, from 690 milliseconds to about 1.07 seconds, and also increased the number of reduceChildren from 21 to 771, but that is significantly better than the original 7234 calls.
| const nestedChildren = | ||
| child.type && isFunction(child.type.getChildren) | ||
| child.type && child.type.role === "stack" && isFunction(child.type.getChildren) |
There was a problem hiding this comment.
Do we get similar performance improvements without this change?
This is a good idea, but we'll have to be a little careful with that since the |
This PR addresses issue #1376.
As soon as the first container component (i.e. Chart, Group, or Stack) renders calls
getCalculatedPropswe effectively have memoized values forcalculatedPropson all subsequent/childgetCalculatedPropscalls (via the props passed in from a parent component) and we were additionally needlessly callingchild.getChildrenin each iteration oftraverseChildrenin each iteration ofreduceChildren. EachgetChildrenthen calledreduceChildren(up to four times) and did the same pattern on its children and so on and so forth. So just defaulting the to current childs children (rather than callinggetChildren) and usingReact.Children.toArrayis enough since we will recurse the whole tree anyways.For the example given in the codesandbox in #1376 I saw an improvement of rendering time from 4.47 seconds to 690 milliseconds and a decrease in calls to
reduceChildrenfrom 7234 to 21.Benchmarks
Before:
After:
