Experimental "all" codepath

[twpayne]

This PR adds a short code path when it's known that all children of a node overlap the target bounding box.

Right now, this code is neither tested nor properly benchmarked. (How do I run the tests?).

Observations:

• In theory, this should improve performance with large bounding boxes by avoiding bounding box intersection tests and instead switching to fast native code (Array.prototype.push.apply) to collapse common cases.
• In practice, the limited testing that I've done indicates that it might not be worth it. I'm not sure why.
• In theory, this should work better with "real life" data where overlapping objects are less common. If objects overlap (e.g. in the debug/perf.js tests) then the advantage will be smaller.

At this stage: comments welcome! Maybe this helps, maybe it doesn't....

[mourner]

That's an interesting suggestion to try, thanks Tom! I'll run the perf tests (I did them mostly with node, node perf.js) and see if it really improves the performance, as it's such a common use case to run large-bbox queries. In theory it should, but V8 can be very smart at optimizing and removing dead code paths.

Tests also leave a lot to be desired, I'll try to improve the coverage significantly as one of the next step improvements.

[mourner]

Here's the culprit! The loop goes through node children, but if the child bbox fits into the search area, you're then running _all on the whole node instead of just the child that fits.

Rewriting this line like this fixes the issue and provides ~3x improvement of 10% searches in my tests:

if (node.leaf) {
    result.push(child);
} else {
    this._all(child, result);
}

[mourner]

I wonder why the tests pass with this. :) There should a mistake there as well.

[twpayne]

Well found @mourner! I've updated the branch to include your fix.

[mourner]

@twpayne Thanks Tom! Any idea why the test passed before the fix?

[twpayne]

I don't know. It may be that it's just a property of the test data that we're using that means that the bug did not manifest in this case. For testing these sort of data structures, I often generate random data and test my code against a known good (but maybe slow, or trivial) implementation, and check that both return the same result.

[twpayne]

Thanks for the second fix :)

[mourner]

@twpayne it would be great to find a test case that fails without the fix but passes in both the latest master and the version before the pull, so we could avoid potential regressions when improving something in future.

[twpayne]

OK, will do. It'll take me a few days (very busy at the moment) but I promise to do it!

[mourner]

No rush Tom, thanks again! This is a really awesome improvement — dramatically better times on all searches (updating benchmark results now).