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

Call determineDataLimits only once #6256

Closed
wants to merge 1 commit into from

Conversation

benmccann
Copy link
Contributor

determineDataLimits is a very expensive call. We call it multiple times for each scale, but really only need to calculate it once

My initial idea was that in creating the layout we should call fit most of the times and update only once, but that proved very difficult. E.g. to fit the scale you need to create the labels, which means you need the format, which in the time scale means you need the unit, which means you need to know how many ticks there are. So the way things are structured right now you can't easily get around building the ticks multiple times. but determineDataLimits is by far the most expensive part of update for both the time and linear scale that I've been testing with, so this is actually a really big win.

@kurkle
Copy link
Member

kurkle commented May 24, 2019

For horizontal axis, the available width could change dramatically between these two calls (from full chart width down to nothing, when vertical axes / legend consumes all the space). In some scales, this changes what ticks are shown and might change the range required to display proper ticks.

Its a good thought, but I think we should calculate data limits separately (only once, and by caching instead of a true/false parameter) and do a lighter "range adjustment" always.

That however might be a breaking change.

@benmccann benmccann mentioned this pull request May 29, 2019
@benmccann
Copy link
Contributor Author

Closing in favor of #6304

@benmccann benmccann closed this Jun 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants