Chart.js
Chart.js copied to clipboard
Improves chart update performance for a large number of datasets
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
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 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 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).
Would love to see this PR move forward
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.
@kurkle Looks better now, Windows build is green, linux not... weird. Is there a chance to see the image diff files?
@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, 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.
@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.