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

Scales are measuring the longest text for all ticks including skipped… #5765

Closed
wants to merge 2 commits into from

Conversation

desean1625
Copy link

… ticks

This causes a decrease in performance.

… ticks

This causes a decrease in performance.
src/core/core.scale.js Outdated Show resolved Hide resolved
Copy link
Author

@desean1625 desean1625 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a codepen that Alerts the time it takes to load the chart. The first alert is the time it takes without the modification, the second is with the scales prototype mutated to include my changes.
https://codepen.io/desean1625/pen/aRVpdj

At least on my browser
Chromium
Version 69.0.3497.81 (Official Build) Built on Ubuntu , running on Ubuntu 18.04 (64-bit)
it is almost 50% less time.

FF 62.0 (64-bit) Similar results.

@benmccann
Copy link
Contributor

@desean1625 will you be able to take another look at this PR in order to call _autoSkip only once?

@desean1625
Copy link
Author

I have not. I will see if I can get some time to look at this again.

@benmccann
Copy link
Contributor

After having spent some time working on the surrounding code and looking at this more, I'm worried this approach won't work because _autoSkip relies on calculateTickRotation having already been called to define labelRotation:

var labelRotationRadians = helpers.toRadians(me.labelRotation);

So we can't call _autoSkip from within calculateTickRotation because labelRotation won't be properly defined

@benmccann
Copy link
Contributor

benmccann commented Feb 26, 2019

As one potential optimization, we are going to stop measuring the label sizes when minRotation === maxRotation. If you're able to set that in your particular use case then that could be an even larger speedup. I'm going to set maxRotation: 0 in my own charts to disable rotation and get the speed boost. I hope that might work for you as well.

@benmccann
Copy link
Contributor

I spent awhile working on this code and talking to the other maintainers about it on Slack. They suggested that the expected order of operations is that we first rotate the ticks and then we do the auto-skipping. I think that this PR doesn't really fit that expectation, which I tried to better document here: #6079. If you look at some of the PRs and issues that are linked to that one you can see more of the discussion that was had and also look in the Slack #dev channel archives from around that date. One of the best ways to speed up your code is to use Chart.js 2.8.0 and set minRotation = maxRotation = yourDesiredRotation. I hope that suggestion and the linked discussions help. I'm going to close this PR since I don't think we can move forward with it. Thanks for the good suggestion here which spurred a lot of the discussion and some other improvements we made to surrounding areas of the code. Appreciate your effort in investigating and suggesting a fix

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

Successfully merging this pull request may close these issues.

2 participants