Skip to content
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

[BUG] determineDataLimits called five times per update #5389

Closed
veggiesaurus opened this issue Apr 4, 2018 · 3 comments · Fixed by #8234
Closed

[BUG] determineDataLimits called five times per update #5389

veggiesaurus opened this issue Apr 4, 2018 · 3 comments · Fixed by #8234

Comments

@veggiesaurus
Copy link
Contributor

Expected Behavior

determineDataLimits is an expensive calculation, and should only be called when needed. If a graph has two axes, determineDataLimits should only be called twice per update.

Current Behavior

determineDataLimits is called five times per update:

  • twice during getMinimumBoxSize
  • twice during fitBox
  • once during finalFitVerticalBox

Possible Solution

Perhaps we could cache the result of determineDataLimits, or ideally refine the chart update lifecycle to not require a scale to be updated at each of the stages mentioned above.

Steps to Reproduce (for bugs)

  1. Create a chart with two axes
  2. Add a dataset to the chart
  3. Update the dataset

Context

determineDataLimits is quite an expensive function. It could also be optimised, but the first step would be to remove duplicate calculations. In my performance tests, each subsequent identical call to determineDataLimits is faster due to JIT optimisations, but this is reliant on the underlying engine to optimise things.

Environment

  • Chart.js version: 2.7.2
  • Browser name and version: Chrome
@etimberg
Copy link
Member

etimberg commented Apr 5, 2018

@veggiesaurus the caching is an interesting idea. @simonbrunel what are your thoughts? I don't think the output of determineDataLimits should change during a single update so caching at some level seems to make sense.

@etimberg
Copy link
Member

etimberg commented Apr 5, 2018

@veggiesaurus perhaps a way to start is with a PR that we can discuss and iterate on. Thoughts?

@veggiesaurus
Copy link
Contributor Author

@etimberg have done so in PR #5393

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.

2 participants