Major Performance Change - Re-Use Expensive To Generate Data

[billneff79]

Recharts, prior to this pull request, wasn't taking full advantage of React's rendering optimizations for sub-trees. The "shallowEqual" function in PureRender was actually a deep-equal, which was very expensive and re-computed several times on any given event (e.g. mouse moving over the graph).

Part of the reason a deep equality was likely used was that some of the props, like the axis maps and ticks were being generated as brand new object on every render, even though the data in them was the same, so a shallow equality would have failed.

In a nutshell, this pull request does two things:

1. modify the shallowEqual function to actually be a shallow equality, only checking one level deep for === equality on all keys in an object
2. For the major charts that had uniformity to how they were rendered (ComposedChart, AreaChart, LineChart, BarChart, PieChart), ANY object that would be expensive to generate when looping over a large dataset (e.g. axismaps, ticks, etc.) is generated only when necessary and stored in state to be passed down to child components as props. Since the objects will pass === checks in this scenario, child components never re-render unless necessary, and determining necessity is cheap now.

All-in-all, it is significantly faster now. lint checks and tests all pass.

Note for future: In running the automated tests and comparing against manual test restuls, I had bugs that I manually found and fixed that weren't failing in any automated tests. We should add test cases for:

• Vertical line to show active index, "recharts-tooltip-cursor", is displaying as it should
• handleBrushChange in generateCategoricalCharts changes the data shown by the underlying chart appropriately

[coveralls]

Coverage decreased (-0.01%) to 92.746% when pulling e22c93f on billneff79:axis-performance into 580ac55 on recharts:master.

[coveralls]

Coverage increased (+0.07%) to 92.826% when pulling 911d18c on billneff79:axis-performance into 580ac55 on recharts:master.

[coveralls]

Coverage increased (+0.1%) to 92.856% when pulling 0638670 on billneff79:axis-performance into 580ac55 on recharts:master.

[coveralls]

Coverage increased (+0.1%) to 92.856% when pulling 82af54d on billneff79:axis-performance into 580ac55 on recharts:master.

[coveralls]

Coverage increased (+0.1%) to 92.894% when pulling bf0e06b on billneff79:axis-performance into 580ac55 on recharts:master.

[coveralls]

Coverage increased (+0.1%) to 92.868% when pulling 7c0c4cb on billneff79:axis-performance into 580ac55 on recharts:master.

[coveralls]

Coverage decreased (-0.1%) to 92.644% when pulling 1e13abb on billneff79:axis-performance into 580ac55 on recharts:master.

[coveralls]

Coverage decreased (-0.3%) to 92.424% when pulling 8cb526e on billneff79:axis-performance into 580ac55 on recharts:master.

[coveralls]

Coverage increased (+0.1%) to 92.868% when pulling 221ba82 on billneff79:axis-performance into 580ac55 on recharts:master.

[billneff79]

I may add more tests, but the bulk of the functionality for this pull request is complete. Please review and merge or give feedback on what you'd like changed.

[coveralls]

Coverage increased (+0.3%) to 93.092% when pulling afbaeb0 on billneff79:axis-performance into 580ac55 on recharts:master.

[billneff79]

I added tests that would have caught my earlier errors:

• Vertical line to show active index, "recharts-tooltip-cursor", is displaying as it should
• handleBrushChange in generateCategoricalCharts changes the data shown by the underlying chart appropriately

[billneff79]

I added tests to future proof against pure rendering getting messed up by checking that mouse enter/move and brush movement that doesn't actually change the start/end index on a LineChart do not cause a re-render of the Line child component.

[coveralls]

Coverage increased (+0.3%) to 93.109% when pulling dfe08f8 on billneff79:axis-performance into 580ac55 on recharts:master.

[coveralls]

Coverage increased (+0.3%) to 93.109% when pulling 8589639 on billneff79:axis-performance into 580ac55 on recharts:master.

[xile611]

@billneff79 Thanks for your contribution. It's really awesome. I'll review all the commits an soon as possible.

[emmenko]

@billneff79 if you fix the conflicts maybe this can go through then?

[billneff79]

I fixed all of the conflicts and all of the tests still pass. I had to artisanally merge the last change from BarChart to get maxSize in, but I think it came out OK ;)

[billneff79]

I added two more commits from our fork that are performance related. I think that's it for these performance changes.

[billneff79]

On a side note, we've effectively abandoned recharts for our project, after we had made some interesting changes to it. You may want to pull them in from our fork. Let me know if you need any help. You can look at the commits after October 3rd, 2016 to get an idea of what we added:

https://github.com/billneff79/recharts/commits/master

Highlights include:

• Allow className attribute to be passed through and added to outside element of ReferenceLine
• Added support for new affectedCharts parameter on Brush component to specify which charts the brush affects (handy if you want to have a context chart that doesn't change while controlling another chart that does)
• Added the ability to overlay a Brush on a chart
• Use globally registered mouse events for the brush for a better UX

[billneff79]

\o/

[xile611]

@billneff79 Thank you for your contribution. It's really awesome.👍 We'll consider about your commits in your master.

[billneff79]

Other performance tips you might want to try for big data sets that we've seen tremendous (4-5x performance boost) results from include:

Don't plot more than one data point per css pixel. i.e. if you have 1000 data points, but your graphable width in your chart is only 300 pixels, you will waste a lot of cpu calculating the path for your chart without being able to see 2/3rds of it. So, we just pruned the data when generating the ticks for our area/line/etc. charts to get it down to 1 point max per pixel: i.e. if there are 3 times as many points as pixels, we generate the graph from data[0], data[3], data[6], data[9], ....

As the user uses a Brush to zoom in on the graph, we re-calculate the data points used for drawing the path, so you can get to full data fidelity on the graph as soon as you zoom into a view where there is at least one point per pixel.

It turns out that it's just way faster to prune the data on each Brush movement than it is not render a full, but overly granular, path

[bluespore]

@billneff79 You are a champion. Thank you for all this insight and your contribution to the repo.

[billneff79]

You're very welcome!

On Tuesday, November 1, 2016, Sean Bullock notifications@github.com wrote:

@billneff79 https://github.com/billneff79 You are a champion. Thank you
for all this insight and your contribution to the repo.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#277 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AP6gZO4UBqL3DNwNZc7OxdVrdoDvSE2zks5q5-0BgaJpZM4KJkqz
.

[mehdivk]

@billneff79 thanks mate!