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

Time scale performance improvement #6019

Merged
merged 6 commits into from
Feb 18, 2019

Conversation

benmccann
Copy link
Contributor

determineLabelFormat was looping through every data point on chart creation and creating a new moment object for each one. This was a major part of the time to render a time chart and was unnecessary. Since it's only used to format the tooltip we can just do it for the corresponding data point when the tooltip is displayed

@kurkle
Copy link
Member

kurkle commented Jan 28, 2019

Nice that you are optimizing!

But, did you realize its going to return different format for different ticks now?
Eg. stamps are
2019-01-01 00:00:00.000, 2019-01-01 00:00:00.500, 2019-01-01 00:00:01.000, .... every 500 ms
First one is going to be formatted by presets.date, second by presets.full, third by presets.time.

In short of removing the whole determineLabelFormat thing, one optimization could be to limit the iteration to a maximum number of points. It could still fail in some scenarios though.

The "remove the whole thing approach" would be to provide less magic out of the box and set just 1 default format - requiring user to change it, if the default does not suit a chart.

@simonbrunel
Copy link
Member

I feel this PR is implementing the same behavior initially rejected in #4583

But, did you realize its going to return different format for different ticks now?

Already discussed in #4583 (comment)

The "remove the whole thing approach" would be to provide less magic out of the box and set just 1 default format

I agree and already suggested: #5095 (comment). What about always falling back to presets.full?

@benmccann
Copy link
Contributor Author

But, did you realize its going to return different format for different ticks now?
Eg. stamps are
2019-01-01 00:00:00.000, 2019-01-01 00:00:00.500, 2019-01-01 00:00:01.000, .... every 500 ms
First one is going to be formatted by presets.date, second by presets.full, third by presets.time.

Yes. I think that's okay. It can be understood as consistently showing the most detail necessary and no more. While not consistent in the format, it's consistent in the logic and always shows a fairly reasonable default.

One optimization could be to limit the iteration to a maximum number of points. It could still fail in some scenarios though.

I wouldn't be opposed, especially in the distribution: 'series' case where ticks are evenly spaced apart like your example. You could look at just a few points and generally have a good idea of what the specific datetime is. It is more error-prone in the distribution: 'linear' case though. But I'm not sure that it should be a requirement to show the same format on every point

What about always falling back to presets.full?

I'd prefer that to the implementation in master, which has a performance degradation. But, in my opinion, it's a bit overkill for a chart showing only annual values. I've attached a screenshot of what that looks like below.

screenshot from 2019-01-28 09-23-27

@kurkle
Copy link
Member

kurkle commented Feb 1, 2019

@benmccann how about defaulting to presets.time?

@benmccann
Copy link
Contributor Author

If you had a chart that was displaying milliseconds then it would show the same tooltip for every data point. And it still seems a bit overkill still for the annual case like the screenshot above

@simonbrunel
Copy link
Member

... the most detail necessary ...

This is based on your expectations, for example (pushing things a bit further), why should we display minutes and seconds for 2019-01-01 04:00:00, 2019-01-01 05:00:00, etc. and not 2019-01-01 4pm? or days for 2019-01-01, 2019-02-01, 2019-03-01, etc. when we could display Jan 2019, Feb 2019, Mar 2019, etc.? So it seems to me that this determineLabelFormat logic implements a use case, not a feature and that's what is wrong here.

... it's consistent in the logic and always shows a fairly reasonable default.

Again, I think this generates a poor default user experience, for example:

- tick 1: 2019-01-01
- tick 2: 2019-01-01 00:00:00.500
- tick 3: 2019-01-01 01:00:00
- tick 4: 2019-01-01 02:00:00.500
- ...

When displaying tooltips point by point, the user will have to "guess" the missing components, I find this quite disturbing while focusing on the moving part (e.g. the hours or, worse, the milliseconds).

But, in my opinion, it's a bit overkill for a chart showing only annual values

Most of the time it will be overridden by the user via tooltipFormat.

@benmccann benmccann force-pushed the determineLabelFormat branch 2 times, most recently from acd515c to 9521956 Compare February 3, 2019 18:06
@benmccann
Copy link
Contributor Author

I don't necessarily agree that the user has to guess the missing part since it's always 0. That being said it's clear that there's agreement not to go with the approach I initially proposed, so I've gone ahead and removed the whole determineLabelFormat function.

@kurkle
Copy link
Member

kurkle commented Feb 4, 2019

I think this is going to the right way.
That original "more info in the tooltip" idea is lost in this PR now.
I still think most users will override that format, so this is about providing most usable default, rather than a perfect solution. ISO-8601 would be a good international default to use, if it was more readable. But its not, so I vote for presets.full

@benmccann
Copy link
Contributor Author

Another idea...

What about presets.full when distribution: 'linear' and timeOpts.displayFormats[me._unit] when distribution: 'series'

presets.full allows you to see the full information if points are placed arbitrarily, but in the series case the points are spaced evenly and so timeOpts.displayFormats[me._unit] shows a more appropriate amount of information

@kurkle
Copy link
Member

kurkle commented Feb 4, 2019

IMO a solid default and no magic is good for this kind of regional option.
OTOH if its not from presets, those can be removed from the adapter. I'd consider that a plus.

@simonbrunel
Copy link
Member

displayFormats[me._unit] is consistent and corresponds to what was requested. The only cons is that it makes this information useless by default, for example if unit is hour the tooltip will display 1AM, 2AM, ...., 1AM, 2AM, ... and so on, without any date information.

IMO a solid default and no magic is good for this kind of regional option.

I agree. I initially thought we should go with a date built-in method (e.g. Date.to*String()) but using one of the adapter presets allows to take in account the upcoming adapter options (i18n, timezone, etc.). So I think preset.full would provide a better default (even if sometime too detailed).

If we go with full, maybe we should move it to the adapter formats, rename it to 'datetime' (for example), and remove presets. This would simplify the API and make 'datetime' configurable. If we don't go with 'full'|'datetime', then we should still remove presets from the adapter API.

I think my preference goes for displayFormats['datetime'] as fallback for the tooltip label. We just need to be sure it doesn't cause trouble to have it as part of displayFormats.

@benmccann
Copy link
Contributor Author

Ok, I've made those changes

@simonbrunel simonbrunel added this to the Version 2.8 milestone Feb 14, 2019
kurkle
kurkle previously approved these changes Feb 14, 2019
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

I think this is a good direction to go.

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

I think you also need to update the formats documentation to ensure that implementers of other adapters know that datetime is expected as key in the dictionary. Perhaps this can be done by updating the unit docs?

@benmccann
Copy link
Contributor Author

@etimberg thanks for taking a look. I updated the docs. Let me know what you think. I didn't update Unit because datetime can't be specified as a scale tick unit and so documented separately

etimberg
etimberg previously approved these changes Feb 14, 2019
nagix
nagix previously approved these changes Feb 15, 2019
kurkle
kurkle previously approved these changes Feb 18, 2019
src/core/core.adapters.js Outdated Show resolved Hide resolved
@benmccann benmccann dismissed stale reviews from kurkle, nagix, and etimberg via 4c85e4b February 18, 2019 18:26
Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

Thanks @benmccann

@simonbrunel simonbrunel merged commit 3e18708 into chartjs:master Feb 18, 2019
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Remove the logic that computed an "optimal" tooltip format. Instead, always fallback to the `datetime` adapter format which is more efficient and stable. Additionally, remove the adapter `presets` API, which is not needed anymore.
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.

5 participants