Skip to content
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

Performance: Avoiding wasteful renders #570

Open
sorenlouv opened this issue Aug 30, 2017 · 8 comments
Open

Performance: Avoiding wasteful renders #570

sorenlouv opened this issue Aug 30, 2017 · 8 comments
Labels

Comments

@sorenlouv
Copy link
Contributor

sorenlouv commented Aug 30, 2017

Use case

I'm adding interaction to a chart, so when the user hovers over the chart, the closes coordinate will "light up".

Example

const data = [
  { x: 1502282820000, y: 0 },
  { x: 1502282880000, y: 480.07448979591834 },
  { x: 1502282940000, y: 210.27743589743585 },
  { x: 1502283000000, y: 437.2161836734694 },
  { x: 1502283060000, y: 178.02835999999996 },
  { x: 1502283120000, y: 462.68808163265305 }
];

class PerfTest extends Component {
  state = {
    markIndex: null
  };

  // Just pseudocode - not called anywhere 
  // this can be achieved with the Voronoi component, 
  // but is out of scope for this example
  onHover(index) {
    this.setState({
      markIndex: index
    });
  }

  render() {
    const { markIndex } = this.state;
    return (
      <XYPlot width={800} height={300} xType="time">
        <XAxis tickTotal={10} />
        <LineSeries xType="time" curve={'curveMonotoneX'} data={data} />

        {markIndex !== null && <MarkSeries data={[data[markIndex]]} />}
      </XYPlot>
    );
  }
}

Performance problem

When this.state.markIndex changes, PerfTest should re-render. However, I've noticed that every child of XYPlot also re-renders. In the example above, I would expect only MarkSeries to be re-rendered. Not XAxis or LineSeries.

Demo

I've created two demonstrations of the problem - one without optimizations, and the other with optimizations. Please allow 5 seconds for the performance test to run, and then check the results in the console.

Codepen without optimizations
https://codepen.io/sqren/pen/WEYYLY?editors=0011

Total render time: 200ms
Wasteful renders: 500
Wasted time: 270ms

Codepen with mutation optimizations
https://codepen.io/sqren/pen/xLQmWL?editors=0011

Total render time: 140ms
Wasteful renders: 250
Wasted time: 60ms

The optimized version makes half the amount of needless renders, and is 30% faster.

Question

I don't think it's a good approach to mutate properties, when passing them to child components. If the component is an instance of React.PureComponent it will not update correctly, so I expect weird results could come out of this.

On the other hand, I do like the performance gains in the optimized version.
What is your take on this?

@akindr
Copy link
Contributor

akindr commented Aug 30, 2017

I've run into similar performance issues with implementing a crosshair on the chart. It's particularly problematic in these spots, where the chart is expected to update at a high FPS but can't keep up because it's re-rendering lines, etc... that haven't changed.

We worked around this by creating two charts that are positioned over each other - one for the crosshair and the other for the line series. Obviously that is a major hack!

It'd be nice if the individual series supported being pure components or even allowed a prop for comparison (say in the case of using Immutable for data storage, you might want to use Immutable.isEqual).

@sorenlouv
Copy link
Contributor Author

Thanks for the advice @akindr! So to understand correctly, did you end up with two XYPlot's positioned absolutely on top of each other?

@akindr
Copy link
Contributor

akindr commented Sep 11, 2017

Yes that is exactly what I did.

@CharukaK
Copy link

Hi, I also have a performance issue regarding a similar situation my use case is a real time dashboard I have noticed that my memory and CPU usage increases with every point added, memory can be handled by limiting the number of points but updating the whole dashboard every second consumes a considerable amount of CPU usage, can you please tell me if there's a way to reduce CPU usage and improve overall performance? Thanks.

@mcnuttandrew
Copy link
Contributor

@CharukaK depending on how many and what type of charts, that might be an SVG issue? If they are mark or line series you can switch those charts to canvas mode by using the canvas versions of those components e.g. MarkSeriesCanvas.

@sorenlouv
Copy link
Contributor Author

sorenlouv commented Sep 18, 2017

@CharukaK I followed @akindr's suggestion and broke the graph into independent React components.

So I actually ended up with four <XYPlot>'s all absolutely positioned on top of each other (with a wrapping container positioned relative).

The most important optimization I did, was to move a Voronoi component into a separate component, that didn't update on every hover interaction. The Voronoi component is quite expensive to re-render - especially if you have many regions, so not something you wanna do several times per second.
You might not use the Voronoi component, but probably use some other components, that are expensive and could benefit from being decoupled from the rest.

To determine which components that were wasting the most time, I used react-addons-perf, and timed it like:

Perf.start();
setTimeout(() => {
  Perf.stop();
  Perf.printWasted();
}, 4000);

(I also setup an automated script to do some work during the first 4 seconds)

@jckr jckr added the WIP label Mar 9, 2018
@originalhat
Copy link

I've created two demonstrations of the problem - one without optimizations, and the other with optimizations.

We ran these again and the performance characteristics don't seem to be that much different. Our guess is that there's been some changes to react that improved deep object comparison. Although after spending some extended time profiling and digging into the react-vis source code, it does still appear that the underlying issue remains. In our case we're looking at building a dashboard of charts, and this small performance bottleneck (re-rendering the entire root) effectively ruins usability.

We've poked around in the react-vis code base, and it appears that there are some fairly complex mechanisms for extracting and injecting props into custom React components as children. Needless to say, it feels like restructuring this code to be optimized for this case may be a great deal of work.

We'd love to hear what ya'll think, and if there might be a good angle of attack for refactoring these component we'd love to help. Otherwise at the very least, an update so we can plan accordingly.

Thanks! <3

@JelleBlaauw
Copy link

I cannot believe this issue has not yet been picked up/resolved. The performance on a stacked bar chart with crosshair is dreadful when highlighting the render updates via the React dev-tools.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants