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.

Sign up for GitHub

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

Jump to bottom

Optimize baseline and special case performance for kSmallest #37

Merged
merged 2 commits into from
Mar 3, 2023
Merged

Optimize baseline and special case performance for kSmallest #37

merged 2 commits into from
Mar 3, 2023

Conversation

Barbarrosa
Copy link
Contributor

Improve the baseline and special case performance of kSmallest.

Before:

starting dynamic queue/enqueue benchmark
FastPriorityQueue--queue-and-kSmallest x 26.71 ops/sec ±1.26% (48 runs sampled)

starting dynamic queue/enqueue benchmark
FastPriorityQueue--queue-and-kSmallest x 2.75 ops/sec ±1.76% (11 runs sampled)

After:

starting dynamic queue/enqueue benchmark
FastPriorityQueue--queue-and-kSmallest x 36.96 ops/sec ±1.03% (64 runs sampled)

starting dynamic queue/enqueue benchmark
FastPriorityQueue--queue-and-kSmallest x 3.66 ops/sec ±1.26% (14 runs sampled)

lemire
lemire reviewed Mar 2, 2023
View reviewed changes
FastPriorityQueue.js
@@ -287,13 +287,22 @@ FastPriorityQueue.prototype.forEach = function(callback) {
FastPriorityQueue.prototype.kSmallest = function(k) {
if (this.size == 0) return [];
k = Math.min(this.size, k);
var fpq = new FastPriorityQueue(this.compare);
const newSize = Math.min((k > 0 ? Math.pow(2, k - 1) : 0) + 1, this.size);
const newSize = Math.min(this.size, (1 << (k - 1)) + 1);
Copy link
Owner

Choose a reason for hiding this comment

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

We had a guard against k == 0 which this PR drops. When k = 0, we get (1<<(-1))+1 which is a silly value.

Copy link
Owner

Choose a reason for hiding this comment

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

If we guard against k == 0 previously, then this code will look safer (k==1 is fine, k < 1 is not).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with the guard you suggested below.

lemire
lemire reviewed Mar 2, 2023
View reviewed changes
FastPriorityQueue.js Outdated
@@ -287,13 +287,22 @@ FastPriorityQueue.prototype.forEach = function(callback) {
FastPriorityQueue.prototype.kSmallest = function(k) {
if (this.size == 0) return [];
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if (this.size == 0) return [];
if ((this.size == 0) || (k<=0)) return [];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented this change after testing. Similar checks I had tested at this point degraded performance, but this did not.

lemire
lemire reviewed Mar 2, 2023
View reviewed changes
FastPriorityQueue.js Outdated
const newSize = Math.min((k > 0 ? Math.pow(2, k - 1) : 0) + 1, this.size);
const newSize = Math.min(this.size, (1 << (k - 1)) + 1);
if (newSize < 2) {
if (newSize < 1) {
Copy link
Owner

Choose a reason for hiding this comment

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

If you add the guard above, then newSize cannot be zero because this.size>0, and (1<<(k-1)+1) is at least 2. So this branch would not be needed.

You could just say if(newSize == 1) { return [this.peek()]; }.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found newSize == 1 appeared to reduce performance, so I went with newSize < 2. This will be safe because this.size and (1 << (k - 1)) + 1 are both guaranteed to be >= 1 when this is checked (excepting cases that were also previously unsupported).

lemire
lemire reviewed Mar 2, 2023
View reviewed changes
FastPriorityQueue.js Outdated
return [this.peek()];
}
}
if (k < 1) return [];
Copy link
Owner

Choose a reason for hiding this comment

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

If you guard at the start for this, as I suggest, then this line is unneeded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

k < 1 check removed.

@lemire
Copy link
Owner

lemire commented Mar 2, 2023

@Barbarrosa This is very good. Please consider my comments.

@Barbarrosa
Copy link
Contributor Author

@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)

@Barbarrosa Barbarrosa requested a review from lemire March 3, 2023 03:55
@lemire
Copy link
Owner

lemire commented Mar 3, 2023

Great. Merging.

@lemire lemire merged commit c571bcf into lemire:master Mar 3, 2023
@Barbarrosa Barbarrosa deleted the minor-ksmallest-improvements branch March 3, 2023 20:22
@staltz
Copy link

staltz commented Mar 8, 2023

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

@Barbarrosa
Copy link
Contributor Author

@staltz I identified the issue, it's an overflow when requesting larger numbers of records. I'll open a PR to fix it.

@Barbarrosa
Copy link
Contributor Author

@staltz @lemire I opened #38 to address the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lemire lemire Awaiting requested review from lemire

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Barbarrosa @lemire @staltz