Skip to content

Constant browser memory growth in Vaadin Charts (specifically, type LINE) #6383

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

Open
jonathanjt opened this issue Jun 19, 2024 · 6 comments · May be fixed by vaadin/web-components#8776
Open

Comments

@jonathanjt
Copy link

Description

Updating an existing chart data series via DataSeries::add( item, true, true ), with Push enabled, causes a growing memory consumption - Chrome DevTools profiler suggests listeners are being added and not being cleaned up when the data points are shifted.

Expected outcome

Would expect memory to remain roughly constant as new data points are added

Minimal reproducible example

v23-chart-leak.zip

Steps to reproduce

Run the provided demo - in creates a chart with some settings ( similar to our production usage where we spotted the issue, in case the particular settings matter ), and sets a timer to update the chart once per second with new data. Shortening the interval will make the growth quite obvious:
image

Environment

Vaadin version(s): 23.5
OS: Linux

Browsers

Chrome

@javier-godoy
Copy link
Contributor

As a lousy workaround , I introduced a call to drawchart(true) every some iterations. It seems to release the listeners.

        AtomicInteger iterations = new AtomicInteger();

        Timer timer = new Timer();
        timer.scheduleAtFixedRate(new TimerTask() {
            @Override
            public void run() {
                if( ui == null ) {
                  return;
                }
                ui.access( () -> {
                    usage_memory_series.add( new DataSeriesItem(
                            Instant.now(), new Random().nextInt(101)), true, true);
                    usage_network_series.add( new DataSeriesItem(
                            Instant.now(), new Random().nextInt(101)), true, true);
                    long t = System.nanoTime();
                    if (iterations.incrementAndGet()==100) {
                        iterations.set(0);
                        usage_chart.drawChart(true);
                    }
                });
            }
        }, 0, 10);

image

OTOH drawChart() which is equivalent to drawChart(false) does not release listeners:
image

@jonathanjt
Copy link
Author

jonathanjt commented Jun 20, 2024

I my testing, using drawChart(true) does release some listeners and free up some memory, but even adding a drawChart(true) after every update still incurs some memory growth, so that slows the process, but doesn't completely work-around the issue.

@DiegoCardoso
Copy link
Contributor

This looks like an issue on Highcharts side, as I was able to reproduce what I believe is the same issue in their latest version.

I reported it here, let's see if they confirm it.

@DiegoCardoso
Copy link
Contributor

As per this comment from one of their maintainers, it has been confirmed that this issue originates on Highcharts.

As usual in this case, we will wait for their fix, and see if it's possible to patch it into our component since Highcharts usually don't backport bug fixes to older versions.

@TatuLund
Copy link
Contributor

I would assume the fix: vaadin/web-components#7785 will give a new workaround to this issue. Instead of just updating Chart data, throw Chart instance away and replace it by new one?

@DiegoCardoso
Copy link
Contributor

The changes in vaadin/web-components#7785 could be used as a workaround for this issue, with the downside that removing and attaching a new chart instance would lose the animation after each new data point is added.

The graph shows that the number of listeners stays fairly consistent after running it for over 5 minutes.

graph

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

Successfully merging a pull request may close this issue.

6 participants