Skip to content

buffer: optimize Buffer#toString()#2027

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

Merged
bnoordhuis merged 1 commit into from
Jun 25, 2015
Merged

buffer: optimize Buffer#toString() #2027

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions benchmark/buffers/buffer-tostring.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
'use strict';

const common = require('../common.js');

const bench = common.createBenchmark(main, {
arg: [true, false],
len: [0, 1, 64, 1024],
n: [1e7]
});

function main(conf) {
const arg = conf.arg;
const len = conf.len | 0;
const n = conf.n | 0;
const buf = Buffer(len).fill(42);

bench.start();
if (arg) {
for (var i = 0; i < n; i += 1)
buf.toString('utf8');
} else {
for (var i = 0; i < n; i += 1)
buf.toString();
}
bench.end(n);
}
13 changes: 11 additions & 2 deletions lib/buffer.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -333,8 +333,7 @@ function byteLength(string, encoding) {

Buffer.byteLength = byteLength;

// toString(encoding, start=0, end=buffer.length)
Buffer.prototype.toString = function(encoding, start, end) {
function slowToString(encoding, start, end) {
var loweredCase = false;

start = start >>> 0;
Expand DownExpand Up@@ -376,6 +375,16 @@ Buffer.prototype.toString = function(encoding, start, end) {
loweredCase = true;
}
}
}


Buffer.prototype.toString = function() {
const length = this.length | 0;
if (length === 0)
return '';
if (arguments.length === 0)
return this.utf8Slice(0, length);
return slowToString.apply(this, arguments);

@mscdexmscdexJun 21, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be faster to do return slowToString.call(this, arguments[0], arguments[1], arguments[2]);? Or maybe pass this as the first argument and avoid .call()/.apply() altogether?

@bnoordhuisbnoordhuisJun 21, 2015

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

See this comment. The initial version called slowToString(this, arguments[0], ...) but when I ran more benchmarks, it turned out that .apply() is faster by about 25-30% once the optimizing compiler kicks in.

@ChALkeRChALkeRJun 22, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Won't function(encoding, start, end) { and return slowToString.apply(this, [encoding, start, end]); work here? There seems to be no reason to use arguments. Could you test that, please?

@mscdexmscdexJun 22, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why create a new array every time when there is already arguments?

@ChALkeRChALkeRJun 22, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok, true, an array is slow.

In my local microbenchmark function(encoding, start, end) { and return .call(this, encoding, start, end) wins for all number of arguments (except three, where .apply(this, arguments) is as fast).

The problem with return slowToString.call(this, arguments[0], arguments[1], arguments[2]); is in arguments, not in .call().

@trevnorristrevnorrisJun 22, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hm. Can't get my test to perform accurately. Seems the true performance hit is using undefined arguments[N] values. Welp, seems we have some cleaning up to do in places like: https://github.com/nodejs/io.js/blob/v2.3.0/src/node.js#L339-L350

@mscdexmscdexJun 23, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@trevnorris What do you mean cleaning up that particular section of code? That code is switching the arguments length and only passing that many arguments.

FWIW I already benchmarked various alternative function calling methods for this patch on top of the next branch (v8 4.3):

Replacing apply() with .call(this, arguments[0], arguments[1], ...) slows down the cases when there are arguments passed, and there is a slight performance hit in the zero argument case (with apply I saw ~510% increase, but call showed ~470%).

Replacing apply() with a direct function call, passing in the context as an extra argument performs about the same as using .call().

Replacing apply() with a switch on arguments.length and using either .call() or passing the context in the < 3 cases (using .apply() as default), the zero argument case is a bit lower IIRC (~470% increase), but now the non-zero argument cases are no longer affected.

So just using .apply() instead of several-line switch is shorter and even a tad faster on the zero argument case. I haven't tested these scenarios on the master branch (v8 4.2) though.

@trevnorristrevnorrisJun 23, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mscdex Is args there an arguments object or an Array? I'm aware that referencing undefined values on an arguments object does have significant overhead, but my benchmarks show that that is not the case for a real array.

@mscdexmscdexJun 23, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@trevnorris I did not test with an array, just arguments.

@trevnorristrevnorrisJun 25, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mscdex neither had I before this PR. Some testing showed that referencing undefined members in an array doesn't have any performance impact. Only side effect is the argument length being too long on the called function.

};

@mscdexmscdexJun 21, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Semicolon can be removed here

@silverwindsilverwindJun 25, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'll look into enabling linter rules for these.

@trevnorristrevnorrisJun 25, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good catch, but not related to or being touched by this PR. Generally we just leave these alone. After the lint rules are enabled a cleanup commit even if it's just this one, would be preferable.

@bnoordhuisbnoordhuisJun 25, 2015

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

FWIW, I included the style fix in the final commit. It didn't make the diff any noisier.



Expand Down