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

fastline controller #6881

Closed
wants to merge 1 commit into from
Closed

fastline controller #6881

wants to merge 1 commit into from

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Dec 31, 2019

I thought I'd share another update since I made a bit of progress since #6822. The main updates are:

  • Code reduction in constrollers.fastline
  • Tooltips are now supported
  • Added _getMinMax for increased performance

I'm not entirely sure the change here tooltip code would be worth adding. I think that if you really care about speed then implementing the tooltip externally would be the way to go. Though the change here would give the user the option and make it more fully featured compared to the normal line controller

The change to make setSyle static could be worth adding since it would save some code in implementing this vs copy-pasting the implementation

TODO: consider implementing fastpath

@kurkle
Copy link
Member

kurkle commented Dec 31, 2019

How does this compare to 'line'? (with animations disable etc)

@benmccann
Copy link
Contributor Author

benmccann commented Dec 31, 2019

On the uPlot benchmark (which has animations disabled):

With line:

  • Chart takes 550 ms
  • initialize takes 130 ms
  • _getMinMax takes 30 ms
  • updateDatasets takes 130 ms
  • draw takes 35 ms

With fastline:

The four affected methods are a total of about 4x faster.

It seems that object creation just really slows things down, which is why both this and #6791 are so impactful. One thing I noticed is the following code in updateElements creates a new object properties per data point:

const properties = {
	x,
	y,
	skip: isNaN(x) || isNaN(y)
};

if (includeOptions) {
	properties.options = me._resolveDataElementOptions(index, mode);
}

me._updateElement(point, index, properties, mode);

I don't fully understand all the new animation stuff, so am not sure if we could just set the properties directly on the Element instead of creating a new object? One way to do it if we need to defer setting the properties might be to do something like:

me._updateElement(point, index, function(element) {
    element.x = x;
    element.y = y;
    element.skip = isNaN(x) || isNaN(y);
    if (includeOptions) {
    	element.options = me._resolveDataElementOptions(index, mode);
    }
}, mode);

@benmccann benmccann force-pushed the fast-line branch 3 times, most recently from ef33420 to 357d856 Compare January 7, 2020 19:26
@benmccann benmccann force-pushed the fast-line branch 2 times, most recently from 99a74b0 to 27d65e6 Compare June 20, 2020 03:49
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