-
Notifications
You must be signed in to change notification settings - Fork 56
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
replace FlatQueue with Heapify? #27
Comments
Nice find — indeed looks like a great library, I didn't know about it! Definitely worth a try, although there are several arguments for keeping
If you make a benchmark that compares the two specifically for Flatbush, let me know — curious to know the results! |
@leeoniya tried this out in the |
@leeoniya experimenting with this further, looks like we can make FlatQueue as fast or even a little faster than Heapify for the Flatbush kNN use case by not shrinking queue arrays on pop/clear — see the PR here mourner/flatqueue#1. Meanwhile we retain the advantage of not allocating more memory than necessary. What do you think? V8 is exceptionally fast on regular arrays nowadays, not that much advantage to typed ones beyond memory footprint. |
sweet!
err, sorry. should not be writing on phone before coffee. smaller mem footprint is always nice, but maybe not if it bloats or complicates the code. if that micro pr has the perf improvements, then perhaps it's good enough. some additional observations: FlatQueue's api seems to use .peekValue() and .peek() for priority. is this atypical for a priority queue where .peek() is usually for value (or key) and something like .peekPriority() is for priority? (this is how heapify does it and maybe more expected). i'm no expert though and my experience here is essentially zero. i think that both FlatBush and FlatQueue can be made smaller by moving away from class and prototype structures and making more variables local within a closure (like uPlot). all of |
@leeoniya no, it's consistent with Heapify, just uses different terms.
I'm not a fan of closures, and find class-based code organization much clearer and easier to read. Also, gzipping strips away most of the |
Fixed in 7a76c8b by upgrading to a newer version of FlatQueue and released as v3.2.1. The memory advantage here is that we don't have to reserve a lot of capacity beforehand, and only use the size needed (which we can't really predict). For the 1 million case in the benchmark, after all kinds of kNN queries, the flat queue uses only ~10k items in average (instead of >1 million). |
yeah, those are definitely the trade-offs. though it's not just this._indices[index]
this._boxes[this._pos++] instead of compressing to this this._indices[a];this._boxes[this._pos++] will compress to x[i];b[p++] comparing just gzip size only accounts for network transfer, but people seem to forget that the whole source still has to be parsed after inflating. i'm sure the difference is not terribly significant for a lib this size, but still worth mentioning. i might make a closure-ified flatbush branch to see the real difference. thanks for the discussion! |
did some probing... after de-prototyping/de-classing Flatbush & FlatQueue, and removing the |
hey @mourner,
this looks quite promising: https://github.com/luciopaiva/heapify
Flatbush already uses typed arrays, so there should be no new compat issues.
looks like it requires knowing the queue capacity in advance, but since Flatbush is static anyhow, this should already be known?
The text was updated successfully, but these errors were encountered: