Buffer performance degradation in 6.6.0

[CurryKitten]

When comparing 6.6.0 against 6.5.0 using one of our benchmarks, we’ve noticed that there’s a performance degradation in the buffer code. We’re running a benchmark that creates a new buffer from an array as many times as it can over a given period of time, for example -

var ARRAY = [1, 2, 23829, 4, 5, 7, 12312321, 2131, 434832, 43792, 23421, 65345, 132210, 77777, 322131, 1, 2, 23829, 4, 5, 7, 12312321, 2131, 434832, 43792, 23421, 65345, 132210, 77777, 322131, 1, 2, 23829, 4, 5, 7, 12312321, 2131, 434832, 43792, 23421, 65345, 132210, 77777, 322131, 1, 2, 23829, 4, 5, 7, 12312321, 2131, 434832, 43792, 23421, 65345, 132210, 77777, 322131];
var ITERATIONS = 300000;
var result;

function test() {
        for(var i=0;i<ITERATIONS;i++) {
                result = new Buffer(ARRAY);
        }
}

On 6.6.0 there an approx 10% slowdown over 6.5.0. Been working through the likely reasons with @gareth-ellis and found that this appears to have been caused by PR #8453 where in buffer.js instances of

if (value instanceof ArrayBuffer)

have been changed to

if (isArrayBuffer(value))

We saw this regression on Linux PPC64, however, if we back this change out of 6.6.0 and rebuild on both Linux PPC64 and Linux Intel, then we see a performance increase, suggesting that the change is having an adverse affect on buffer performance.

[addaleax]

I’ll look into this. I assume the overhead comes from the explicit call into C++, so it trades of correctness for performance; I guess this particular case could be easily improved with an Array.isArray() shortcut, but I’ll try to make the Buffer benchmarking suite a bit more comprehensive to get an accurate picture of how Buffer.from() performs.

[addaleax]

Benchmark PR for Buffer.from(): #8738

[trevnorris]

I threw up a branch w/ a commit that should gain back that performance issue: trevnorris@e555b5a

not putting that in a PR just yet because it doesn't fix the actual problem. was just able to find performance elsewhere that was lying around.

[addaleax]

btw, #8739 touches the same code, so you may want to comment there too, once you know more?

[Fishrock123]

fix is in #8754 I think?

[addaleax]

No, that is a distinct performance regression –#8754 fixes a performance regression that comes with the V8 5.4 upgrade, nothing that’s released yet.

[Trott]

Is this still an issue?

[apapirovski]

I'm going to go ahead and close given the lack of any updates or movement in over a year.