buffer: faster case for create buffer from empty string #4414

JacksonTian · 2015-12-24T07:22:08Z

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

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

mscdex · 2015-12-24T09:21:15Z

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 · 2015-12-24T09:52:15Z

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

jbergstroem · 2015-12-26T11:31:32Z

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

trevnorris · 2015-12-28T17:48:16Z

lib/buffer.js

@@ -105,6 +105,10 @@ function fromString(string, encoding) {
    encoding = 'utf8';

  var length = byteLength(string, encoding);
+
+  if (length === 0)
+    return new Buffer(0);


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

@JacksonTian can you address this?

I prefer use new with constructor.

I do, too. I think there's even a linter rule in place for it. Are you fine with it @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 · 2015-12-28T17:48:30Z

One comment but LGTM.

jasnell · 2016-01-15T01:43:28Z

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

jasnell · 2016-03-22T04:34:50Z

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

JacksonTian · 2016-03-23T13:18:34Z

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 · 2016-03-23T13:35:20Z

benchmark/buffers/buffer_zero.js


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


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 (let i = 0; i < n * 1024; i++) Buffer.from(zeroBuffer); else if (conf.type === 'string') for (let i = 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.

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

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 mean the conf.type === 'buffer' will be run much times？

在 2016年3月23日，下午9:53，Ben Noordhuis <notifications@github.com mailto: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

When create Buffer from empty string will touch C++ binding also. This patch can improve edge case ~70% faster.

JacksonTian · 2016-03-23T14:48:34Z

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 · 2016-03-23T15:31:23Z

LGTM if @bnoordhuis is happy and CI is green

jasnell · 2016-03-23T15:31:59Z

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

When create Buffer from empty string will touch C++ binding also. This patch can improve edge case ~70% faster. PR-URL: #4414 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2016-03-27T16:15:34Z

CI was green. Landed in afd821a

When create Buffer from empty string will touch C++ binding also. This patch can improve edge case ~70% faster. PR-URL: #4414 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

evanlucas · 2016-03-31T14:24:04Z

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

Apparently, the following no longer throws an error:

Buffer('', 'buffer')

evanlucas · 2016-03-31T14:38:14Z

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 · 2016-03-31T14:40:43Z

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 · 2016-03-31T14:43:19Z

Sounds good to me. Thanks!

mscdex added the buffer Issues and PRs related to the buffer subsystem. label Dec 24, 2015

JacksonTian force-pushed the edge branch from 1bdb82e to 70a617c Compare December 24, 2015 09:49

jasnell added the lts-watch-v4.x label Dec 25, 2015

Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:01

trevnorris reviewed Dec 28, 2015
View reviewed changes

JacksonTian force-pushed the edge branch from 70a617c to faf8339 Compare December 31, 2015 10:05

ChALkeR added the performance Issues and PRs related to the performance of Node.js. label Feb 16, 2016

JacksonTian force-pushed the edge branch 2 times, most recently from 514aa5a to 03579ba Compare March 23, 2016 13:27

bnoordhuis reviewed Mar 23, 2016
View reviewed changes

buffer: faster case for create buffer from empty string

de8f36d

When create Buffer from empty string will touch C++ binding also. This patch can improve edge case ~70% faster.

JacksonTian force-pushed the edge branch from 03579ba to de8f36d Compare March 23, 2016 14:48

jasnell closed this Mar 27, 2016

evanlucas mentioned this pull request Mar 31, 2016

v5.10.0 Release Proposal #5970

Closed

evanlucas added the dont-land-on-v5.x label Mar 31, 2016

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Apr 8, 2016

theanarkh mentioned this pull request Aug 17, 2022

[v18.x backport] lib: add diagnostics channel and perf hooks detail #44256

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buffer: faster case for create buffer from empty string #4414

buffer: faster case for create buffer from empty string #4414

JacksonTian commented Dec 24, 2015

mscdex commented Dec 24, 2015

JacksonTian commented Dec 24, 2015

jbergstroem commented Dec 26, 2015

trevnorris Dec 28, 2015

silverwind Jan 11, 2016

JacksonTian Jan 11, 2016

silverwind Jan 11, 2016

trevnorris Jan 11, 2016

trevnorris commented Dec 28, 2015

jasnell commented Jan 15, 2016

jasnell commented Mar 22, 2016

JacksonTian commented Mar 23, 2016

bnoordhuis Mar 23, 2016

JacksonTian Mar 23, 2016

bnoordhuis Mar 23, 2016

JacksonTian Mar 23, 2016

JacksonTian commented Mar 23, 2016

jasnell commented Mar 23, 2016

jasnell commented Mar 23, 2016

jasnell commented Mar 27, 2016

evanlucas commented Mar 31, 2016

evanlucas commented Mar 31, 2016

jasnell commented Mar 31, 2016

evanlucas commented Mar 31, 2016

buffer: faster case for create buffer from empty string #4414

buffer: faster case for create buffer from empty string #4414

Conversation

JacksonTian commented Dec 24, 2015

mscdex commented Dec 24, 2015

JacksonTian commented Dec 24, 2015

jbergstroem commented Dec 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevnorris commented Dec 28, 2015

jasnell commented Jan 15, 2016

jasnell commented Mar 22, 2016

JacksonTian commented Mar 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacksonTian commented Mar 23, 2016

jasnell commented Mar 23, 2016

jasnell commented Mar 23, 2016

jasnell commented Mar 27, 2016

evanlucas commented Mar 31, 2016

evanlucas commented Mar 31, 2016

jasnell commented Mar 31, 2016

evanlucas commented Mar 31, 2016