Conversation
mourner
left a comment
mourner
left a comment
There was a problem hiding this comment.
Overall this is a really awesome improvement! Thanks for a great contribution. 👍
|
Also wondering: is this increase for smaller queries a fluke due or is there an overhead?
|
I would say both is possible here. The current benchmark is not sophisticated enough to track this down. Overall the time for small queries seems to scatters signicantly with every run. |
After some more benchmarking there seems to be an apparent overhead if the number of rectangles in the search area are low. Edit: After some more debugging this seems to be also present if the relevant code is unreachable which indicates that it is related to the javascript engine or javascript timers not being precise in the first place. |
|
@muendlein might be worth increasing the number of searches between timings for a possibly more reliable measurement. Does the last commit help? I'll likely land this anyway eventually since it's an obvious net positive, just wanted to explore whether we could minimize the hit on smaller queries. |
@mourner The last commit has only been a minor improvement which doesn't close the gap. |
One idea is to set an empyrical threshold of the query area compared to data bounds, over which we'll apply the "all in query bounds" logic, and under which we'll leave the existing logic. The overhead for calculating that area for a single query should be small. |
Edit: At least the topic of unreachable functions can be solved with a separate function. @mourner Already tried something similar which does not have an impact. All my testing points towards JIT compiler issues. To make it more clear here are two unreachable examples. Example 1: This is exactly the old logic apart from the additional Before: This PR: Unreachable example 1: Example 2: Same as example 1 but with the majority of the logic removed inside the unreachable if statement. Now you can see that the performance of this example is the same as before. Right now I don't have any good explanation for this except for certain compiler optimizations. Before: This PR: Unreachable example 2: |
|
@muendlein I've seen similar behavior before, and my guess is that it's because of v8 inlining. Over a certain threshold of complexity or size, v8 stops inlining the function, which makes it slower for small payloads. Might be worth experimenting with cutting out some of the logic into a separate function so that most of the hot path code in |
@mourner Just tested this (see my edit above), at least it will fix the topic of unreachable code paths. But for now I only see small improvements in the "real" scenario. |
|
With the latest commit the gap has been reduced but is still there. |
|
@muendlein this one looks much better!
|
|
@mourner Inlining What exactly is your idea about the heuristic approach to check the relative query size? Especially for unbalanced distributions I'm not sure if this is even feasible. |
|
@muendlein so, if I do fb78a2e and then put |
I assume you mean This brings me to the next point, namely how create an empirical yet reliable yet fast |
It's high compared to the case when we guess right; however it's very small compared to not landing this PR. So we need to decide what's better: accept a guaranteed notable performance drop for small queries (arguably the more prevalent case in real world apps than big queries), or accept that we'll sometimes guess wrong and the performance will be as before. I'd probably try the simplest metric possible, e.g. |
|
@mourner I think you are asking the right question about what is the better choice here. Given the varity of datasets & query usecases, I think 1 solution fits all won't exist. For example your proposed simple metric can work great for well distributed datasets but may yield undesired performance in case of strongly unbalanced datasets. Threshold -> 0: always run the optimized path (best performance for large queries) This ensures that users themselves can optimize search performance for their respective dataset. |
|
👍 to exposing an option if the regression is unavoidable. "mouse hover single data point in a scatterplot of 300k points" is something i would prefer not to regress. |
@leeoniya Even with the regression, I'm pretty sure you won't observe any difference as it relative not absolute. When talking about hovering there are much slower processes involved. |
|
I'm hesitant about introducing such an option, because I'd like to keep the library simple, minimal and working perfectly out of the box, and this parameter is pretty confusing and difficult to explain. I think it's fine if there are some weird edge cases with heavily imbalanced datasets where the optimization doesn't kick in, as long as the library as a whole performs great most of the time. So I'd still try to explore the heuristical approach, if there are no other ideas on how to address the small query regression. |
|
@mourner I just tested the simplest heuristics approach and it seems like we are back fighting the compiler. Basically as soon as |
|
@muendlein all right, let this sit for a few days more, I'll try to play with it a bit... As a last resort, we could just add a duplicate method, e.g. |
|
@mourner As some time has passed, I'm wondering if you already had time to play around? |
|
@muendlein sorry, just got around to looking again. Fiddled a bit — seems like performance is fine after reusing the bbox values in the check (see the merge commit), but let's measure again on your bigger benchmarks. Also, this PR needs to be updated to accommodate the change in #68 (passing leaf bbox values to |
|
@mourner I have now updated the PR include the filterFn logic in the leaf function. Before (main): After (this PR): |
|
@muendlein yeah, but it seems like it's a much smaller overhead than before, right? I'd love to see some more detailed benchmarks like the ones you did above. |
|
@mourner I'm not sure if the overhead changed much. Here is the complete benchmark: |
|
@mourner I have now switched to a recursive implementation which seems to close the gap significantly. For larger datasets it is actually consistently ahead. I guess this should be good enough, what do you think? |
|
Excellent! A few more nits:
|
|
@mourner Moved the function now (did not observe performance degradation). |
|
@mourner Is there anything else needed to get this merged? |
|
@muendlein sorry for dropping the ball on this, it's been a very tough winter for me. I'll take another look soon — I've been hesitating because while the PR improved a lot with iterations, it's still a notable regression for the most common use case (arguably, most performance-sensitive apps using flatbush depend on many small queries rather than few big ones). Will take one last look into whether we can get it closer to original performance for that case. |
|
@mourner Thank you for coming back to me and hopefully things are getting better with spring around the corner! I see the point with smaller queries being the more common use case. But for those cases isn't the number of items also large (like >1M)? So far the benchmarking indicates that the regression is mainly present for small queries on small datasets. |
|
I just repeated the benchmark (with more focus on smaller queries) and noticed that the most recent commit (function moved to class improved performance a bit). Regression is now only observable with <=250k items. |
|
@muendlein apologies again for taking so long. I just fed the PR to Codex 5.4, and it suggested an interesting idea: only apply the full bbox optimization on higher leaf levels. It claims this eliminates most of the small query regression while keeping the large query win. Can you try it out? Here's the diff. Also let's rebase on main and resolve conflicts (there were some minor typing changes there). diff --git a/index.js b/index.js
index fb8f602..dc3068d 100644
--- a/index.js
+++ b/index.js
@@ -223,7 +223,7 @@ export default class Flatbush {
/** @type number[] | undefined */
const results = [];
- this._searchRecursive(minX, minY, maxX, maxY, results, nodeIndex, filterFn);
+ this._searchRecursive(minX, minY, maxX, maxY, results, nodeIndex, this._levelBounds.length - 1, filterFn);
return results;
}
@@ -235,10 +235,11 @@ export default class Flatbush {
* @param {number} maxY
* @param {number[]} results
* @param {number} nodeIndex
+ * @param {number} level
* @param {(index: number, x0: number, y0: number, x1: number, y1: number) => boolean} [filterFn] An optional function that is called on every found item; if supplied, only items for which this function returns true will be included in the results array.
* @returns {void}
*/
- _searchRecursive(minX, minY, maxX, maxY, results, nodeIndex, filterFn) {
+ _searchRecursive(minX, minY, maxX, maxY, results, nodeIndex, level, filterFn) {
const end = Math.min(nodeIndex + this.nodeSize * 4, upperBound(nodeIndex, this._levelBounds));
// search through child nodes
@@ -255,12 +256,12 @@ export default class Flatbush {
const index = this._indices[pos >> 2] | 0;
- if (nodeIndex >= this.numItems * 4) {
+ if (level > 0) {
// check if node bbox is completely inside query bbox
- if (minX <= x0 && minY <= y0 && maxX >= x1 && maxY >= y1) {
+ if (level > 1 && minX <= x0 && minY <= y0 && maxX >= x1 && maxY >= y1) {
this._addAllLeavesOfNode(results, pos, filterFn);
} else {
- this._searchRecursive(minX, minY, maxX, maxY, results, index, filterFn);
+ this._searchRecursive(minX, minY, maxX, maxY, results, index, level - 1, filterFn);
}
} else if (filterFn === undefined || filterFn(index, x0, y0, x1, y1)) {
results.push(index); // leaf item |
# Conflicts: # index.js
3 participants
Fixes issue: #60
before
1000 searches 100%: 34.013s
1000 searches 75%: 25.787s
1000 searches 50%: 16.775s
1000 searches 25%: 9.530s
1000 searches 10%: 3.225s
1000 searches 1%: 280.145ms
1000 searches 0.01%: 11.675ms
after
1000 searches 100%: 18.888s
1000 searches 75%: 13.866s
1000 searches 50%: 9.127s
1000 searches 25%: 5.771s
1000 searches 10%: 1.963s
1000 searches 1%: 208.407ms
1000 searches 0.01%: 15.793ms