Improves chart update performance for a large number of datasets #11836
Conversation
|
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). |
from perf doc : https://github.com/chartjs/Chart.js/blob/b9c01414bac867310d192da676c78e8e269f7d8b/docs/general/performance.md?plain=1 PARSING: we can optimize by removing parsing, if we send date in the valid format and ordered, we can set parsing to false and save some time following the doc: https://www.chartjs.org/docs/latest/samples/bar/stacked-groups.html and testing with (array of object (x, y) instead of array of int) testing at odoo with parsing = false, there is a small problem, i'm still looking why the count is not well displayed (fyi, it's well computed as you can see in the y axis) Data normalization when parsing, i used unique and sorted data, so it should work with normalized: true Decimation (https://github.com/chartjs/Chart.js/blob/b9c01414bac867310d192da676c78e8e269f7d8b/docs/configuration/decimation.md) can't be acheived as our labels are not linear or time, we use stages (string) Disable Animations for large data i'm also wondering if we disable the view for large data. it doesn't make sense to show tens of thousands of data points on a graph that is only a few hundred pixels wide. it was crushing, and sometimes loaded in 160 seconds, now it's done in around ~ 70 seconds, but as you can see it's not readable at all I also found an open issue on github, and someone tried to solve it https://github.com/chartjs/Chart.js/pull/11836 it's still not approved i will need to test on a staging env (real data) to be coherent.
|
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 Second, I just can't get the tests running in Intellij. They only run via node script (e.g. 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? |
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
Fixes #11814 partly