Closed Barbarrosa closed 1 year ago
@Barbarrosa This is very good. Please consider my comments.
@lemire I updated the branch based on your comments. Here is the updated performance based on my testing today.
Before (slow)
starting dynamic queue/enqueue benchmark
FastPriorityQueue--queue-and-kSmallest x 25.80 ops/sec ±0.42% (46 runs sampled)
starting dynamic queue/enqueue benchmark
FastPriorityQueue--queue-and-kSmallest x 2.70 ops/sec ±1.92% (11 runs sampled)
Original PR (faster)
starting dynamic queue/enqueue benchmark
FastPriorityQueue--queue-and-kSmallest x 39.60 ops/sec ±0.95% (52 runs sampled)
starting dynamic queue/enqueue benchmark
FastPriorityQueue--queue-and-kSmallest x 4.06 ops/sec ±0.80% (15 runs sampled)
PR after changes (similar to Original PR)
starting dynamic queue/enqueue benchmark
FastPriorityQueue--queue-and-kSmallest x 39.81 ops/sec ±0.67% (53 runs sampled)
starting dynamic queue/enqueue benchmark
FastPriorityQueue--queue-and-kSmallest x 4.03 ops/sec ±1.56% (15 runs sampled)
Great. Merging.
Just a heads up (I don't have more information at the moment), this PR broke a corner case in jitdb, something about calling kSmallest
and it returning undefined. I'll try to work on finding a simple reproduction and submit a test to FastPriorityQueue.
cc @arj03
@staltz I identified the issue, it's an overflow when requesting larger numbers of records. I'll open a PR to fix it.
@staltz @lemire I opened https://github.com/lemire/FastPriorityQueue.js/pull/38 to address the issue.
Improve the baseline and special case performance of
kSmallest
.Before:
After: