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

Small optimizations #6868

Merged
merged 2 commits into from
Jan 3, 2020
Merged

Small optimizations #6868

merged 2 commits into from
Jan 3, 2020

Conversation

kurkle
Copy link
Member

@kurkle kurkle commented Dec 29, 2019

  • Only extend element if configuration is provided
  • Time scale lookup, check index validity before accessing table[-1]

A 20 times sequential uPlot (parsing: false) comparison:
Master (average ~125ms/chart):
image
image

MR (average ~105ms/chart):
image
image

@@ -139,7 +139,7 @@ function lookup(table, key, value) {

while (lo >= 0 && lo <= hi) {
mid = (lo + hi) >> 1;
i0 = table[mid - 1] || null;
i0 = mid > 0 && table[mid - 1] || null;
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to me like this can only happen when lo and hi both equal 0. Is that right? Why do we need a special check for that? It would seem the code comment (given value is outside table (before first item)) no longer applies in that case

I think we should get rid of the interpolation stuff entirely, which would be much faster and simpler. We can instead check the distribution to be a lot more efficient. Something like:

getPixelForValue(value) {
	const me = this;
	if (distribution === 'series') {
		return getPixelForDecimal(me._timestamps.indexOf(value) / me._timestamps.length);
	} else {
		return getPixelForDecimal((value - me.max) / (me.min - me.max));
	}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This happens when lo = 0 and hi = 1. (0 + 1) >> 1 = 0
indexOf would be a lot slower than the lookup table.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another problem with indexOf is that you have to have all the timestamps in place to be able to get a pixel for it.
For example:

data: [{x: 1577659670, y: 1}, {x: 1577659680, y: 2}]

getPixlForValue(1577659675) // undefined 

Copy link
Contributor

@benmccann benmccann Dec 30, 2019

Choose a reason for hiding this comment

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

Yeah, you're right that indexOf would be a O(n) lookup. We could use a Map instead to make it O(1). Especially since a Map is already being created (since #6871 - though we'd need the value to be the index rather than true).

I don't think that we'd ever call getPixelForValue for a value that's not in the dataset provided by the user, so I'd be fine with explicitly not supporting that as I don't see a use case for it.

src/core/core.element.js Show resolved Hide resolved
@kurkle kurkle requested a review from benmccann January 1, 2020 07:14
@kurkle kurkle added this to the Version 3.0 milestone Jan 1, 2020
@etimberg etimberg merged commit 099e552 into chartjs:master Jan 3, 2020
@kurkle kurkle deleted the optimize branch February 19, 2020 18:32
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.

3 participants