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.

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

benchmark: add benchmark for buf.compare() #5441

Closed
wants to merge 3 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Feb 26, 2016

There is a benchmark for the class method Buffer.compare() but not for
the instance method buf.compare(). This adds that benchmark.

I used this to confirm a performance regression in an implementation I
was considering. While the implementation was a bust, it does seem like
the benchmark is worthwhile.

The benchmark is nearly identical to the existing Buffer.compare()
benchmark except, of course, that it calls buf.compare() instead.

There is a benchmark for the class method `Buffer.compare()` but not for
the instance method `buf.compare()`. This adds that benchmark.

I used this to confirm a performance regression in an implementation I
was considering. While the implementation was a bust, it does seem like
the benchmark is worthwhile.

The benchmark is nearly identical to the existing `Buffer.compare()`
benchmark except, of course, that it calls `buf.compare()` instead.
@Trott Trott added buffer Issues and PRs related to the buffer subsystem. benchmark Issues and PRs related to the benchmark subsystem. lts-watch-v4.x labels Feb 26, 2016
var b1 = new Buffer(size).fill('a');

b1[size - 1] = 'b'.charCodeAt(0);

Copy link
Contributor

Choose a reason for hiding this comment

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

Please insert code to force optimization of b0.compare before starting the benchmark. Take a look at some of the other (e.g. path) benchmarks to see how to do this.

@Trott
Copy link
Member Author

Trott commented Feb 26, 2016

  • const-ified per @evanlucas
  • added a force-optimization (or tried to, copy-pasting mostly) per @mscdex

@evanlucas
Copy link
Contributor

LGTM

b1[size - 1] = 'b'.charCodeAt(0);

// Force optimization before starting the benchmark
b0.compare(b0, b1);
Copy link
Contributor

Choose a reason for hiding this comment

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

This and the other instance below should instead be b0.compare(b1);

@Trott
Copy link
Member Author

Trott commented Feb 26, 2016

@mscdex
Copy link
Contributor

mscdex commented Feb 26, 2016

LGTM

@Trott
Copy link
Member Author

Trott commented Feb 28, 2016

Trott added a commit to Trott/io.js that referenced this pull request Feb 28, 2016
There is a benchmark for the class method `Buffer.compare()` but not for
the instance method `buf.compare()`. This adds that benchmark.

I used this to confirm a performance regression in an implementation I
was considering. While the implementation was a bust, it does seem like
the benchmark is worthwhile.

The benchmark is nearly identical to the existing `Buffer.compare()`
benchmark except, of course, that it calls `buf.compare()` instead.

PR-URL: nodejs#5441
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
@Trott
Copy link
Member Author

Trott commented Feb 28, 2016

Landed in ffdc046

@Trott Trott closed this Feb 28, 2016
@Fishrock123 Fishrock123 mentioned this pull request Mar 1, 2016
5 tasks
Fishrock123 pushed a commit that referenced this pull request Mar 2, 2016
There is a benchmark for the class method `Buffer.compare()` but not for
the instance method `buf.compare()`. This adds that benchmark.

I used this to confirm a performance regression in an implementation I
was considering. While the implementation was a bust, it does seem like
the benchmark is worthwhile.

The benchmark is nearly identical to the existing `Buffer.compare()`
benchmark except, of course, that it calls `buf.compare()` instead.

PR-URL: #5441
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 17, 2016
There is a benchmark for the class method `Buffer.compare()` but not for
the instance method `buf.compare()`. This adds that benchmark.

I used this to confirm a performance regression in an implementation I
was considering. While the implementation was a bust, it does seem like
the benchmark is worthwhile.

The benchmark is nearly identical to the existing `Buffer.compare()`
benchmark except, of course, that it calls `buf.compare()` instead.

PR-URL: #5441
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
There is a benchmark for the class method `Buffer.compare()` but not for
the instance method `buf.compare()`. This adds that benchmark.

I used this to confirm a performance regression in an implementation I
was considering. While the implementation was a bust, it does seem like
the benchmark is worthwhile.

The benchmark is nearly identical to the existing `Buffer.compare()`
benchmark except, of course, that it calls `buf.compare()` instead.

PR-URL: #5441
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
@Trott Trott deleted the benchmark-instance branch January 13, 2022 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. buffer Issues and PRs related to the buffer subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants