Chart.js icon indicating copy to clipboard operation
Chart.js copied to clipboard

Improves chart update performance for a large number of datasets

Open jwedel opened this issue 1 year ago • 9 comments

This PR improves the chart update performance drastically by replacing a nested loop O(n^2) by an index access O(1) in a method that is called a lot during chart update.

On a reference system (Mac M1 Pro), the fix enables

  • 10.000 datasets instead of 2.500 with acceptable performance
  • including zooming, panning and hovering

Fixes #11814 partly

jwedel avatar Jul 17 '24 15:07 jwedel

The tests are failing, I will have look. The problem ist, that I have a little trouble due to this issue ( #11828 ) as I always have test failures and thus its is hard to spot which ones are related to my change.

jwedel avatar Jul 18 '24 11:07 jwedel

@jwedel the tests that are failing don't seem to be image based tests. It looks like they're failing in some specific cases when datasets are inserted

etimberg avatar Jul 18 '24 12:07 etimberg

@etimberg yes, I know.

The problem is, that I ran the tests on clean master and had 2 failing tests.

Then, implemented the fix and also got 2 failed tests.

I just realized, that karma stops on the first failing test, and it runs on two browsers, so just looking at the failing test number does not help in my special case (as I assumed).

jwedel avatar Jul 18 '24 14:07 jwedel

Would love to see this PR move forward

NotTsunami avatar Oct 17 '24 08:10 NotTsunami

Would love to see this PR move forward

Hi, me, too.

But I have to be honest, this code base is really giving me a hard time.

First, I had this problem that it only did show the first error and stops. Therfore, I temporarily added stopOnSpecFailure: false which then showed me all the existing failures.

Second, I just can't get the tests running in Intellij. They only run via node script (e.g. test-ci-karma). When starting the test in IntelliJ, I get:

Karma v 6.4.1 - connected; test: karma_error You need to include some adapter that implements __karma__.start method!;

This is so annoying, I can only use fit/fdescribe to run single test, and worst thing, I can't debug the code. I tried lots of console debugging but it is a huge waste of time. I don't know if no one uses debugging or if my local set-up is just broken.

If anyone could help me getting a setup (preferably in IntelliJ/WebStorm) to debug single test cases, then I would try to continue getting this PR done.

The current problem is, that there some edge cases that do not work anymore. E.g. removing or shuffeling datasets. This is something we don't do in our application without rebuilding this whole chart from scratch so this "issue" does not affect us but it let the tests fail. I started implementing a rebuildMetasets method that is called once during update to handle all the cases but something breaks and I am blind on this.

Short disclaimer: I'm off on vacation from tomorrow and will be able to continue in 8-9 days, if anyone is able to help.

jwedel avatar Oct 23 '24 22:10 jwedel

@kurkle Looks better now, Windows build is green, linux not... weird. Is there a chance to see the image diff files?

jwedel avatar Nov 18 '24 21:11 jwedel

@kurkle kindly asking if you could have a look, why only the linux build failed. Seems like a tiny difference change in the image based tests:

[karma] 	  Difference: 26416px / 10.08%
[karma] 	  Threshold: 10%
[karma] 	  Tolerance: 0.1%

I don't know how the difference looked like before.

Is there a way to view the image difference so we can vusally check that it is functionally correct or if something has actually changed for the worse?

jwedel avatar Dec 02 '24 13:12 jwedel

@jwedel, I'm trying to test this build inside my app. Do you happen to have it published as an npm package somewhere?

A manual build does apparently not play well with the realtime streaming plugin I use.

vbackeberg avatar Mar 13 '25 15:03 vbackeberg

@jwedel, I'm trying to test this build inside my app. Do you happen to have it published as an npm package somewhere?

A manual build does apparently not play well with the realtime streaming plugin I use.

No, we use monkey patching the functions as we don’t want to maintain our own builds for chartJS.

jwedel avatar Mar 13 '25 16:03 jwedel