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

buffer: improve Buffer.byteLength(string, encoding) #1441

Closed
wants to merge 1 commit into from

Conversation

JacksonTian
Copy link
Contributor

When string is empty, it will running into binding also.
It make the performance is wasted.

@JacksonTian
Copy link
Contributor Author

It is an edge case too.

Benchmark test case is:

var Benchmark = require('benchmark');
var suite = new Benchmark.Suite();

suite
.add("byteLength empty string", function () {
  Buffer.byteLength('');
})
.add("byteLength long string", function () {
  Buffer.byteLength('test');
})
// add listeners
.on('cycle', function (event) {
  console.log(String(event.target));
})
.on('complete', function () {
  console.log('Fastest is ' + this.filter('fastest').pluck('name'));
})
.run({ async: false });

Result is here:

io.js $ ./iojs ~/git/lab/murmurhash/byte_length.js 
byteLength empty string x 601,367,144 ops/sec ±1.43% (93 runs sampled)
byteLength long string x 5,804,314 ops/sec ±1.69% (94 runs sampled)
Fastest is byteLength empty string
io.js $ iojs ~/git/lab/murmurhash/byte_length.js 
byteLength empty string x 6,169,601 ops/sec ±0.96% (98 runs sampled)
byteLength long string x 5,801,272 ops/sec ±1.05% (92 runs sampled)
Fastest is byteLength empty string

for empty string, about 100x times improved.

case 'binary':
case 'raw':
return string.length;
if (string.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just do if (string.length === 0) return 0; here? That would trim down the diff a bunch. :)

@Fishrock123 Fishrock123 added the buffer Issues and PRs related to the buffer subsystem. label Apr 16, 2015
@cjihrig
Copy link
Contributor

cjihrig commented Apr 16, 2015

The diff would be a lot cleaner if you did this before the switch.

if (string.length === 0)
  return 0;

@JacksonTian
Copy link
Contributor Author

thanks @Fishrock123 and @cjihrig , updated.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 16, 2015

In your benchmark results, where is the performance improvement coming from on non-empty strings?

@JacksonTian
Copy link
Contributor Author

@cjihrig the difference of two non-empty strings result maybe in 1% deviation. maybe can be ignored.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 16, 2015

Yea, likely just noise. LGTM, but the if would look better split across two lines :-).

@JacksonTian
Copy link
Contributor Author

updated.

When string is empty, it will running into binding also.
It make the performance is wasted.
@Fishrock123
Copy link
Contributor

LGTM

@fengmk2
Copy link
Contributor

fengmk2 commented Apr 16, 2015

+1

@tellnes
Copy link
Contributor

tellnes commented Apr 16, 2015

LGTM

Fishrock123 pushed a commit that referenced this pull request Apr 16, 2015
When the string is empty, calling the binding is unnecessary and slow.

PR-URL: #1441
Reviewed-by: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Christian Tellnes <[email protected]>
@Fishrock123
Copy link
Contributor

Thanks, landed in 431673e

@JacksonTian JacksonTian deleted the improve_byteLength branch December 17, 2015 03:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants