Improves chart update performance for a large number of datasets #11836
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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). |
Draft
RaoufGhrissi
added a commit
to odoo-dev/odoo
that referenced
this pull request
Aug 14, 2024
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 chartjs/Chart.js#11836 it's still not approved i will need to test on a staging env (real data) to be coherent.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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