plouc / nivo

nivo provides a rich set of dataviz components, built on top of the awesome d3 and React libraries
https://nivo.rocks
MIT License
13.24k stars 1.03k forks source link

Performance issue with several charts (regression in v0.73) #1740

Open petrvecera opened 3 years ago

petrvecera commented 3 years ago

Describe/explain the bug When doing re-render (change of the input data) of the legend in the Bar chart, it's extremely slow (several seconds). Waiting for: 'requestAnimationFrame' handler took <N>ms

This behavior is best spotted when you have bigger chart ~30 items.

When you do first render it's super fast. Only on data change it's slow. It happens only with Legend change, when you change the chart data (values) it's still fast.

We have not tested any other charts than Bar Chart - as we don't use them with big data.

We have view with 2 big charts - on data input change it takes ~5s for them to re-render. image

To Reproduce Example code https://codesandbox.io/s/perfomrance-issue-16vsp?file=/src/index.tsx The only thing it does is randomize the legend values on button click.

Steps to reproduce the behavior:

  1. Go to https://codesandbox.io/s/perfomrance-issue-16vsp?file=/src/index.tsx
  2. Open dev tools
  3. Click on button "Click me"
  4. Observe the long wait

Expected behavior The Chart is refreshed fast.

Desktop (please complete the following information):

Regression The chart works fast with version 0.72 the performance issue is introduced in version 0.73

plouc commented 3 years ago

I've updated the example a little bit, to make sure the problem doesn't come from top re-renders: https://codesandbox.io/s/perfomrance-issue-forked-018kh?file=/src/index.tsx, and it appears to only happen when the value of indexBy (bulletinName) is changed.

On a side note, nivo makes heavy use of hooks, so in your example when you're doing this:

.map((field) => {
    field.bulletinName = (Math.random() + 1).toString(36).substring(5);
    return field;
})

You're returning the same ref for each data point, which can lead to issues, but it's not the cause of the perf issue.

plouc commented 3 years ago

I think I found the issue, now react-spring does not unmount components after a leave transition, we'll have to configure the expires property to solve this.

FiftyToo commented 3 years ago

I believe this affects the sunburst chart as well.

plouc commented 3 years ago

@FiftyToo yes, it affects several packages.

christo9090 commented 3 years ago

Just wanted to jump in here and say I am having the same problem. I have a button which changes the keys to display a different set of data and I also get 40 warnings or so of "requestAnimationFrame". Also, the GUI is completely frozen until all the warnings run their course.

plouc commented 3 years ago

@christo9090, did you try the latest version?

spmacdonald commented 3 years ago

I appear to be having the same issue on version @nivo/bar@0.74.0

Thank you for the great library, I have been enjoying using it.

christo9090 commented 3 years ago

@plouc Yes unfortunately I am on version 0.74.0 for @nivo/bar and @nivo/core. I see the updates to react-spring in the dependencies as well. Every time I change the key for the graph, I get the "requestAnimationFrame" warning exactly twice as many times as there are bars in the graph. It also slows the page down quite a bit, in some cases none functional until all the warnings finish popping up. Happy to provide any more information.

The library is amazing by the way. Really love it.

image

TomasB commented 2 years ago

@plouc still an existing issue in 74.1 which prevents us to upgrade, and meanwhile we are stuck on 72.0, which has other issues (like crashing ResponsiveLine when there are no data series) Actually, to be fair, the issue is slightly different - we see that it reverses the order of the data array, not array of series.

shmidla commented 2 years ago

@plouc This is repeated at 0.79.1. Data total 6 series in 30 days = total 180 values. After clicking on an element in the legend, the chart is redrawn and then freezes for 4-5 seconds. After the time expires, the legend becomes available again, as well as tooltips and so on. The performance shows that the problem is in react-spring. It's funny that during the performance recording, the freeze time is reduced to 1.5 seconds. This issue reproduces in all recent browsers. image

When the animation is disabled, it is similar, only the feeling of the frieze increases, since the presence of animation still smooths out this effect a little.

npirotte commented 2 years ago

Hi, is there any update on that issue? I'm running into the same thing, using @nivo/bar 0.79.1

nathancharles commented 2 years ago

I'm also seeing this same issue for @nivo/sunburst 0.79.1

tkonopka commented 2 years ago

Hi @petrvecera, @plouc, and all. Just adding some observations. I can see the lag in the original sandbox, but I could reproduce it only partially in a from-scratch setup with current package versions.

In the sandbox below, the chart is quite responsive to data replacement and toggling with the legend. It seems to work reasonably well even with 200 bars.

https://codesandbox.io/s/lucid-montalcini-blqv50

The sandbox runs with React 18. Curiously, it runs much slower in React 17 mode, especially the legend toggling. Copying the same code into my local environment (also React 17) is slower still because of the warnings reported by @christo9090, and overall the experience is as described by @shmidla.

dgoemans commented 2 years ago

Had the same issue on 0.79.1 for svg bar and scatterplots. I ended up switching to the canvas versions as the issue does not persist there (although you miss out on the nice transitions)

GuilhermeGuitte commented 2 years ago

Hello, is there any update on that issue? I'm also running into the same issue, using @nivo/bar 0.79.1

animalxxv commented 2 years ago

I found a workaround, I have been experiencing the same issue when switching chart data using ResponsiveBar from @nivo/bar 0.79.1 with react 17.0.2

Workaround

Use: import { ResponsiveBarCanvas } from '@nivo/bar'; Instead of: import { ResponsiveBar } from '@nivo/bar';

Not tested with other chart types (pie, line, etc.). Using Canvas versions of the chart types will probably work too.

Problem when I use ResponsiveBar

DevTools console messages (level=verbose) image

animalxxv commented 2 years ago

Had the same issue on 0.79.1 for svg bar and scatterplots. I ended up switching to the canvas versions as the issue does not persist there (although you miss out on the nice transitions)

Just saw your comment after posting my comment... I can confirm the workaround works for me

wes commented 1 year ago

The workaround works well for me :)

sebastienbarre commented 1 year ago

I can't see these warnings anymore as of today, but ResponsiveBar (SVG) is still so much slower than ResponsiveBarCanvas. And it's not even a ton of SVG elements, only 96 bars, but it's slow (compared to other libraries I've tried for the same amount of data).

plouc commented 1 year ago

@sebastienbarre, would you mind sharing your benchmarks? Also do those other libraries provide the same features?

I'd like to add that It's not just about the number of bars, for example, ticks, grids, labels... also add DOM nodes, while canvas doesn't and interactivity is handled via a single event listener for the canvas implementation, while you have one on each bar for the SVG version.

And if you found faster alternatives, no pb, good for you, but I think it's best to be specific about what you compared (and share the metrics) before making this statement.

plouc commented 1 year ago

Updated one of the previously provided sandbox: https://codesandbox.io/s/intelligent-thunder-xme4cz?file=/src/App.js.

plouc commented 1 year ago

Also please make sure to measure the performance of the chart with a production build of react, for example the sandboxes are typically running a dev build.

sebastienbarre commented 1 year ago

I'm comparing Nivo to Highcharts here, as we are definitely trying to move away from Highcharts. I believe my problem is closer to #1826 which the author solved by... switching to the canvas implementation, like others did in this thread. Which would be fine by me, except that some features are missing from the Canvas implementation, such as Markers.

I have to evaluate multiple charting libraries right now, since this choice will impact years to come on a new project. We are starting with Nivo, which is a very nice library -- lots of pluses, and a few minuses that I need to take into account to, out of fairness, such as the absence of Markers in the Canvas implementation, or problematic auto-alignment when dealing with tooltips.

The chart I'm testing is not very complex, here it is below with Highcharts (SVG). Essentially a time series, 6 data points per month for the first 4 "months", then 15+ for each of the next 12 months.

image

The above is a stacked area chart that we want to change to a stacked bar chart anyway, so here it is at the moment in Nivo, and it uses less data (6 data points per "month" for 16 months):

image

Note the selector above, which allows us to select a specific data point out of the 6 data points available for each month, to focus on that slice of the data more specifically. In this example below, the bottom bar of each stack is now on its own:

image

The issue I'm experiencing ONLY happens when switching from ALL (aka 6 data points per month) to ONE data point. There is a massive slowdown occurring before the new version of the charts appears, which is strange because it contains much less data. However, this does NOT happen when switching from ONE data point to ANOTHER data point. Or from ONE data point back to ALL data point. It only occurs going from ALL to ONE. The component is only re-rendered once, I tested that out. So I'm wondering if this is because of the clean-up necessary to get rid of the SVG elements that have to be removed when switching from ALL to ONE, since there is much less data to display (1/6th). This is with a prod build of React.

This entirely goes away when switching to the Canvas implementation (but no Markers). SVG is generally slower than Canvas... but we have the same selector for the Highcharts version (which uses SVG), and no slowdown to report when switching from ALL to NONE.

plouc commented 1 year ago

@sebastienbarre, thank you for the detailed explanation, the issue you describe is indeed very strange, but this gives me an idea for what to look at.

sebastienbarre commented 1 year ago

Thanks! This also gave me some ideas. At first I suspected that maybe Highcharts was re-using some SVG elements, but no, they do not appear to have unique ids to rely upon. Then after looking at what was going on more closely, I realized the comparison is not entirely fair: Nivo is re-creating the chart entirely when going from ALL to ONE. Highcharts is not, I just noticed that it is setting the visibility to "hidden" on all the elements it had previously drawn, and only modifying the coordinates for the subset of data point being selected by the ONE choice. So this technically is a lot less work.

I'm going to try to do the same in Nivo, aka the data prop should not change, but only the keys would. UPDATE: nope, that didn't solve the slowdown.

plouc commented 1 year ago

This is a very different approach, @react-spring can be configured to never unmount elements (via expires), but I guess it can generate quite some garbage in certain situations (for example a rolling histogram), as I understand it, elements wouldn't be unmounted, but they couldn't be reused for arbitrary keys (not exactly a free pool). But that's something I could try at least, to see if the issue comes from the unmounting/mounting.

sebastienbarre commented 1 year ago

Thanks, I appreciate you taking a look! Note that I'm using animate={false}, so I would have expected react-spring not to be involved at all?

plouc commented 1 year ago

The initial approach when I created the lib (and using react-motion) was to have 2 distinct implementations, but this was a lot of duplication, and when you already have 3 variants (SVG, HTML, canvas for some charts), it's a lot to maintain and to install, so I wanted to reuse the same code, which seemed feasible by using immediate={true}.

sebastienbarre commented 1 year ago

I see. Now curiously enough, the sandbox you just updated here seems to have a lot more data than I do in my chart, and yet clicking on "Update data" doesn't show the slowdown I see. Then again it is generating about the same amount of data each time...

humanchimp commented 1 year ago

please excuse my drive-by comment: does e.g. generateBars need a memo? (i would look but i was having trouble getting this repo to play nice with volta)

joaopedromatias commented 1 year ago

I was facing the issue of having my JS main thread being blocked when the state of data changed. I was able to solve it with:

setChartPageElement(<></>)
setTimeout(() => {
    setChartPageElement(chartPageElement)
}, 0)

This way the does not re-render, but dies for another one to be born.

carlos-duetto commented 9 months ago

I am seeing this issue as well with scatterplot using react-spring/web v 9.6.1. Do you know if this issue persists in the most recent version of react-spring/web or most recent version of nivo (0.84)?

OleksandrRakovets commented 4 months ago

@carlos-duetto yes, it is still there on 0.87

OleksandrRakovets commented 4 months ago

Experiencing this issue with very few amount of bars: going from 20 to 5 bars significantly freezes the whole page (not only the chart) up to 5 seconds

OleksandrRakovets commented 4 months ago

Hi @plouc!

Can you please point to the file that is suspected to cause an issue?

It is a major blocker for us, so I can try to fix it.

Krizz1983 commented 1 month ago

Thanks for an amazing chart Library 😀🤩

I found a workaround to the issue as well. Though it is not a good workaround I thought I shared it anyway.

OnFilter()=>{ setChartDate([]); setTimeout(()=>setChartDate(data), 100); } <Bar data={chartData} width={props.width} height={props.height} ... />