Skip to content

Conversation

@benmccann
Copy link
Contributor

ticksFromTimestamps is pretty expensive in the uPlot benchmark since it creates an object for every tick. Leaving ticks as primitives is much more performant and will be one less thing for the user to migrate since that's how it is in 2.9 for all scales other than the time scale. This now stores the major attribute in a separate _tickMeta array of objects, which is much cheaper since an object only needs to be created when major: true

buildTicks timing
Before: 73.05 ms
After: 19.23 ms

This makes the total render time for the Chart about 10% faster

@kurkle
Copy link
Member

kurkle commented Nov 28, 2019

Did not review yet, but in general I think ticks should be a small number of items, and fine being objects (maybe even elements)

I think there is something to fix elsewhere.

Some generic thoughts:

  • This is mainly a time scale issue.
  • linear mode could always generate the ticks between min and max, that would ensure sane number of ticks (unless user sets too small unit).
  • series mode could generate ticks from labels, using every nth index to generate desired amount of ticks. User could provide the labels, if the outcome is not desirable. Edit: user provided label would need to have a index too, to place it in the correct position.
  • we could explore some sort of "break" or "gap" configuration for scales, to allow weekend exclusion in linear mode. Related: How to add scale breaks to a chart? #6737

@benmccann
Copy link
Contributor Author

Simon had suggested the current approach of making buildTicks relatively dumb and putting the bulk of the logic in autoSkip instead which was implemented in #6509. I agree it's mainly a time scale issue at the moment though if we were to continue following the vision that Simon laid out then the linear scale, logarithmic scale, etc. would also have really straightforward logic for buildTicks and would rely more heavily on autoSkip instead as well.

series mode could generate ticks from labels, using every nth index to generate desired amount of ticks

It sounds like basically we would want that to result in essentially the same outcome as source: 'labels' (or equivalently source: 'data') and autoSkip: true does today. I had actually experimented with doing something like this in buildTicks when implementing #6509, but a big issue was that buildTicks would then need to figure out how many ticks to place based on the label size and axis length. The issue is that in buildTicks not all of these values have been figured out yet because fit is called after buildTicks which changes the axis length.

One thing I don't like right now is how much both the core scale's _autoSkip and individual scales' buildTicks need to know about the label size and axis length. E.g. scale.core._tickSize vs scale.time._getLabelSize vs scale.linear._computeTickLimit. There's a lot that's duplicated and things tend to get buggy when the logic in buildTicks differs from the logic in autoSkip. And plus buildTicks can't do it accurately since it doesn't know the axis length with margin/padding taken into account. I don't know if maybe there's some better way to rethink the interplay between buildTicks, calculateTickRotation, fit, and autoSkip.

@benmccann
Copy link
Contributor Author

benmccann commented Dec 31, 2019

@kurkle any thoughts on this? I can't really come up with any other way to improve performance here

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.

Couple of notes. Lets keep this open for a while still, I haven't had the time to take a look in depth yet.
This could be the way to go with timeSeries scale, if splitting them up makes sense. For linear mode I think it is going to be more efficient to generate the labels from min/max only.

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.

2 participants