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

Use binary search for interpolations #6958

Merged
merged 3 commits into from
Jan 21, 2020
Merged

Conversation

kurkle
Copy link
Member

@kurkle kurkle commented Jan 14, 2020

Closes: #4246

master
pr

@kurkle kurkle changed the title Binary search Use binary search for interpolations Jan 14, 2020
etimberg
etimberg previously approved these changes Jan 14, 2020
@kurkle
Copy link
Member Author

kurkle commented Jan 14, 2020

Master is not that bad now, because #6953 was already merged :)

if (!element.skip) {
handler(element, metaset.index, index);
const {index, data} = metasets[i];
const {lo, hi, loIndex, hiIndex} = lookup(data, axis, position[axis]);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why this is returning two items. Shouldn't the data be located just at one index? It seems like handler could be called extra times and after the time scale is split we may not need the "lo" and "hi" concept but then still be using it here

Copy link
Member Author

Choose a reason for hiding this comment

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

image

Event at the black dot, those highlighted indices are returned. This is not strictly needed here, but the cost is insignificant.

@kurkle
Copy link
Member Author

kurkle commented Jan 15, 2020

With caching of dimensions:

var start = Date.now();
var rect = chart.canvas.getBoundingClientRect();
for (var i = 0; i < 100000; i++) {
	var ix = Math.round(Math.random() * 55549);
	var point = chart._metasets[0].data[ix];
	var event = {
		type: 'mousemove',
		target: chart.canvas,
		clientX: rect.left + point.x,
		clientY: rect.top + point.y,
		cancelable: true,
		bubbles: true,
		view: window
	};
	chart.getElementsAtEventForMode(event, 'nearest', {intersect: false, axis: 'x'});
}
console.log(Date.now() - start);

master: ~750ms
pr: ~370ms

@@ -112,7 +109,7 @@ function getIntersectItems(chart, position, axis) {
}
};

const optimized = evaluateItemsAtIndex(chart, axis, position, evaluationFunc);
const optimized = binarySearchItems(chart, axis, position, evaluationFunc);
Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized this might not technically be correct (eventhough I introduced it). Imagine you have very large points that are all overlapping with the given position. Just returning the items at the nearest index might not be enough because there could be elements at several indices that are all intersecting with the given point

Copy link
Member Author

@kurkle kurkle Jan 16, 2020

Choose a reason for hiding this comment

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

Thats right, index and (plain) coordinate based determination can both be wrong.
I'll remove the intersect optimization for now.
This case needs the generalized, will do that in another pr.
This case could be optimized using ranged lookup, but would need maximum radius/dimension on the axis direction of the elements to know how large range needs to be handled.

Something like:

let range = determineMaxSizeOfElementsForAxis(axis);
const start = lookup(data, axis, position[axis] - range);
const end  = lookup(data, axis, position[axis] + range);
const startIndex = start.loIndex || start.hiIndex;
const endIndex = end.hiIndex || end.loIndex;
for(let i = startIndex; i <= endIndex; ++i) {
  handler(data[i], index, i);
}

@kurkle
Copy link
Member Author

kurkle commented Jan 16, 2020

Ok, ready for review again.

  • Removed the caching, because its not that simple. Deserves its own PR.
  • If elements share options, use binarySearch for intersect mode too (other conditions still apllying)
    • assuming the size of the elements is consistent with shared options.

Timing: ~950ms (master is still ~750ms)

Change mode to 'linear', and reduce loop count to 1000 (so it completes in master):
pr: 21ms
master: ~6000ms (a lot of variation)

src/core/core.datasetController.js Outdated Show resolved Hide resolved
src/core/core.interaction.js Outdated Show resolved Hide resolved
src/core/core.interaction.js Outdated Show resolved Hide resolved
@kurkle
Copy link
Member Author

kurkle commented Jan 16, 2020

Reverse mode was not working, so fixed that.
https://plnkr.co/edit/PpmJrvIakA01hzgbnwQq?p=preview

Revert generalization

Revert getDistanceMetricForAxis relocation

Private

Add underscore

Cache dimensions

Remove optimization of getIntersectItems

Revert "Cache dimensions"

This reverts commit dc0252d.

Conditionally use binarySearch for intersect mode

Simplify lookup, add reverse support

Intersect
@etimberg etimberg merged commit 9fda5ec into chartjs:master Jan 21, 2020
@etimberg etimberg added this to the Version 3.0 milestone Jan 21, 2020
@kurkle kurkle deleted the binarySearch branch February 19, 2020 18:25
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.

[BUG] Hover is slow in line graphs when there are many points
4 participants