buffer: faster case for create buffer from empty string

[JacksonTian]

When create Buffer from empty string will touch
C++ binding also.

This patch can improve edge case ~70% faster.
following is benchmark results.

[mscdex]

I'm confused, so it got slower after this patch? It went from 2767 ops/sec to 605 ops/sec and 2599 ops/sec to 1249 ops/sec?

[JacksonTian]

@mscdex I updated the results. the buffer result is noisy because I used old version(not lastest version) to run it.

[jbergstroem]

CI: https://ci.nodejs.org/job/node-test-commit/1533/

[trevnorris]

new is unnecessary anymore since it forces a return of a new Uint8Array.

[silverwind]

@JacksonTian can you address this?

[JacksonTian]

I prefer use new with constructor.

[silverwind]

I do, too. I think there's even a linter rule in place for it. Are you fine with it @trevnorris?

[trevnorris]

@silverwind The code works fine, but we're allocating an unnecessary object. Though I doubt that has an impact on performance either. Part of the point is that Buffer is not an actual constructor anymore. It's now more of a factory method. But again it's nothing serious or is a change that needs to be made.

[trevnorris]

One comment but LGTM.

[jasnell]

New CI, the last one appears to have died completely: https://ci.nodejs.org/job/node-test-pull-request/1246/

[jasnell]

@JacksonTian ... can you rebase this and update the new Buffer() usage to use the new constructor APIs (Buffer.alloc(0))

[JacksonTian]

Hi, @jasnell , the branch rebased.

./node benchmark/compare.js ./node ~/git/node/node -- buffers buffer_zero
running ./node
buffers/buffer_zero.js
running /Users/jacksontian/git/node/node
buffers/buffer_zero.js
buffers/buffer_zero.js n=1024 type=buffer: ./node: 1824.6 /Users/jacksontian/git/node/node: 1842.9 . -0.99%
buffers/buffer_zero.js n=1024 type=string: ./node: 1901.3 /Users/jacksontian/git/node/node: 956.59 . 98.76%

[bnoordhuis]

The change to lib/buffer.js LGTM but I wonder if it makes a difference on the benchmark numbers when you hoist out the conditional like this:

if(conf.type==='buffer')for(leti=0;i<n*1024;i++)Buffer.from(zeroBuffer);elseif(conf.type==='string')for(leti=0;i<n*1024;i++)Buffer.from(zeroString);

Also, I'm not sure if crankshaft will constant-fold the expression n * 1024 away, you might want to check if caching it makes a difference.

[JacksonTian]

I think it's unrelated. these code is running in difference process when benchmarking it.

[bnoordhuis]

Sorry if I'm being unclear. What I mean is that the overhead of the check and the multiplication may affect the benchmark results. They probably don't but it would be good to check.

[JacksonTian]

You mean the conf.type === 'buffer' will be run much times？

在 2016年3月23日，下午9:53，Ben Noordhuis <notifications@github.commailto:notifications@github.com> 写道：

In benchmark/buffers/buffer_zero.js #4414 (comment):

function main(conf) {
var n = +conf.n;
bench.start();
for (let i = 0; i < n * 1024; i++) {

• Buffer.from(zero);
• conf.type === 'buffer' ? Buffer.from(zeroBuffer) : Buffer.from(zeroString);
  Sorry if I'm being unclear. What I mean is that the overhead of the check and the multiplication may affect the benchmark results. They probably don't but it would be good to check.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub https://github.com/nodejs/node/pull/4414/files/03579ba47a711923c67215d63385b60cdf473c98#r57161722

[JacksonTian]

Hi, @bnoordhuis I update the benchmark script. following is result:

./node
buffers/buffer_zero.js
running /Users/jacksontian/git/node/node
buffers/buffer_zero.js
buffers/buffer_zero.js n=1024 type=buffer: ./node: 2019.3 /Users/jacksontian/git/node/node: 2039.4 . -0.99%
buffers/buffer_zero.js n=1024 type=string: ./node: 1935.6 /Users/jacksontian/git/node/node: 1136.6 . 70.29%

[jasnell]

LGTM if @bnoordhuis is happy and CI is green

[jasnell]

New CI: https://ci.nodejs.org/job/node-test-pull-request/2042/

[jasnell]

CI was green. Landed in afd821a

[evanlucas]

Hm, it looks like this is failing on v5.x right now.

Apparently, the following no longer throws an error:

Buffer('', 'buffer')

[evanlucas]

It looks like master has https://github.com/nodejs/node/blame/master/lib/buffer.js#L211 and v5.x does not.

I'm not sure if that was intentional or not, but I'm a little weary of including this in v5.x for now. I'm going to leave it out of v5.10.0 and we can reevaluate

[jasnell]

Given that the change was an additional throw (and therefore semver major),
it was intentionally left out of the backport to v5.
On Mar 31, 2016 7:38 AM, "Evan Lucas" notifications@github.com wrote:

It looks like master has
https://github.com/nodejs/node/blame/master/lib/buffer.js#L211 and v5.x
does not.

I'm not sure if that was intentional or not, but I'm a little weary of
including this in v5.x for now. I'm going to leave it out of v5.10.0 and we
can reevaluate

—
You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub
#4414 (comment)

[evanlucas]

Sounds good to me. Thanks!