buffer: Preallocate array with buffer length in Buffer#toJSON

[alemures]

Because the final array length is known, it's better to allocate its
final length at initialization time to avoid future reallocations.

It also adds an explicit buffer length greater than 0 comparison so
it's more readable, avoids the internal ToBoolean call and follows the
standard Node.js API format (as it can be checked in other similar
structures where 'length > 0' is preferred over 'length')

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

buffer

[mscdex]

I think I'm -1 on this. See this relevant comment. It's not immediately clear what branches this change affects (negatively) performance-wise, as I'm not sure which (if any) have had the appropriate patches backported from upstream V8.

[alemures]

There is actually a benchmark created in the last update of this function "benchmark/buffers/buffer-tojson.js". It could be useful to know if this change have had some performance improvement, I'll be able to get some results in a while.

[ChALkeR]

Note that the commit message does not follow commit guidelines atm — the subsystem is not present.

[alemures]

Here I have some interesting results from the benchmark mentioned before showing up to 3 times more operations for the bigger buffer:

master

buffers/buffer-tojson.js len=0 n=10000: 5,084,450.175184731
buffers/buffer-tojson.js len=10 n=10000: 3,875,130.882545558
buffers/buffer-tojson.js len=256 n=10000: 593,240.6046854025
buffers/buffer-tojson.js len=4096 n=10000: 42,223.39550717062

Actual PR

buffers/buffer-tojson.js len=0 n=10000: 5,515,017.115855619
buffers/buffer-tojson.js len=10 n=10000: 4,850,801.473673488
buffers/buffer-tojson.js len=256 n=10000: 1,448,223.8258887387
buffers/buffer-tojson.js len=4096 n=10000: 149,254.77835441532

[mscdex]

I just checked the results on each versioned branch and master, and it does provide about the same performance or faster in each case, so LGTM.

[ChALkeR]

The commit message still needs to be fixed =).

[alemures]

I'll get the commit message fixed shortly, I suppose it can be done for already pushed commits.

[alemures]

I just fixed the commit message adding the subsystem and fixing a couple of spelling mistakes.

Apart from that, I have been doing tests removing the "if(this.length > 0)... else" and i am getting exactly the same results when the buffer's length is 0. If there is no other reason why we are using this if statement other than performance, I will suggest to remove it so the code will look more polished and simple.

[TimothyGu]

I will suggest to remove it so the code will look more polished and simple.

Can you do that while at it?

[alemures]

I just ran the benchmark without the if...else statement and the results are exactly the same so, I suppose that it doesn't affect performance.

buffers/buffer-tojson.js len=0 n=10000: 5,342,776.512727028
buffers/buffer-tojson.js len=10 n=10000: 5,032,158.005735653
buffers/buffer-tojson.js len=256 n=10000: 1,420,578.24069056
buffers/buffer-tojson.js len=4096 n=10000: 144,037.6943189128

[jasnell]

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

[jasnell]

@alemures ... can you look at #11733 (comment), thank you!

[alemures]

@jasnell I proposed a small style change to this PR but I'm not sure if you guys agree or not. I suppose that it's better to leave as it is now that CI reported no errors with the actual code. Do you think it is still worth it the change?

[jasnell]

I'm good with this as is. Will get it landed.

[jasnell]

landed in 51587b2

[MylesBorins]

@mscdex are you ok with the way this change landed? I'm seeing the original -1 but no sign off

Also curious if this should be backported

[mscdex]

@MylesBorins It would need to be benchmarked first, I'm not sure the performance fix for new Array(len) was ever backported to older branches. Without that, these changes will make it slower (compared to initializing with []).