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.

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

buffer: improve creation performance #6893

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
10 participants
@RReverser
Member

RReverser commented May 20, 2016

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

buffer

Description of change

Improves performance of allocating unsafe buffers, creating buffers from
an existing ArrayBuffer and creating .slice(...) from existing Buffer by
avoiding deoptimizing change of prototype after Uint8Array allocation
in favor of ES6 native subclassing.

This is done through an internal ES6 class that extends Uint8Array and
is used for allocations, but the regular Buffer function is exposed, so
calling Buffer(...) with or without new continues to work as usual
and prototype chains are also preserved.

Performance wins for .slice are +120% (2.2x), and, consequently, for
unsafe allocations up to +95% (1.9x) for small buffers, and for safe
allocations (zero-filled) up to +30% (1.3x).

chart

@RReverser

This comment has been minimized.

Member

RReverser commented May 20, 2016

cc @trevnorris as per request

@targos

This comment has been minimized.

@RReverser

This comment has been minimized.

Member

RReverser commented May 20, 2016

CI finished, I see only two failures in Windows build configs which are network timeouts. I guess unrelated?

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented May 20, 2016

Hasn't Trevor already changed this between JS and C++ like 3 times? 😂

@RReverser

This comment has been minimized.

Member

RReverser commented May 20, 2016

@Fishrock123 Well, it's mostly in JS, but the inheritance from Uint8Array was done inefficiently. I just additionally removed CreateFromArrayBuffer in C++ as it's not needed when you have native subclass of Uint8Array which can be already instantiated with an ArrayBuffer as an argument (and do that faster).

TL;DR: the biggest win here is unrelated to the C++ change, but rather to the ES6 native subclassing.

@jasnell

This comment has been minimized.

Member

jasnell commented May 20, 2016

@eljefedelrodeodeljefe

This comment has been minimized.

Contributor

eljefedelrodeodeljefe commented May 21, 2016

I like the changes very much. Also makes it more readable...

I'll test around .slice since I conducted some experiments and found out that v8 is already really fast there. I am curious whether there is an regression now.

@RReverser

This comment has been minimized.

Member

RReverser commented May 22, 2016

v8 is already really fast there

Well, now Buffer.slice is very close to Uint8Array.subarray (I was comparing against it in my local benchmarks as a "theoretical maximum" which obviously can't be achieved in a wrapper, but can be very close (and it is now)).

Please do let me know if I missed something / need to change before this can be merged.

@@ -213,7 +214,7 @@ function allocate(size) {
// Even though this is checked above, the conditional is a safety net and

This comment has been minimized.

@ChALkeR

ChALkeR May 22, 2016

Member

Side note: this comment is irrelevant now, probably since dd67608, I overlooked it.
/cc @trevnorris

This comment has been minimized.

@trevnorris

trevnorris May 22, 2016

Contributor

Looks like it can be removed.

This comment has been minimized.

@RReverser

This comment has been minimized.

@ChALkeR

ChALkeR Jun 3, 2016

Member

Looks like it has returned after a rebase =).
It's not critical, though, that could be removed later.

return b;
}
function fromArrayBuffer(obj, byteOffset, length) {
if (!isArrayBuffer(obj))
throw new TypeError('argument is not an ArrayBuffer');

This comment has been minimized.

@addaleax

addaleax May 22, 2016

Member

Basically, this error can only be raised if someone’s messing with the prototype of obj to make it look like an ArrayBuffer, right?

If the others in @nodejs/buffer agree I’d suggest dropping this check then.

This comment has been minimized.

@RReverser

RReverser May 22, 2016

Member

Yes, initially I didn't put it in here, but there's a test against it: https://github.com/nodejs/node/blob/master/test/parallel/test-buffer-arraybuffer.js#L39-L46

This comment has been minimized.

@addaleax

addaleax May 22, 2016

Member

That test even passes without this check now, the expected TypeError is thrown when accessing byteLength below, although with a different message (which would technically make dropping this a semver-major change…).

I assume the isArrayBuffer check was only added in the first place to avoid crashes on the C++ side, but that doesn’t seem to be a concern anymore if everything’s done in JS.

This comment has been minimized.

@RReverser

RReverser May 22, 2016

Member

the expected TypeError is thrown when accessing byteLength below, although with a different message (which would technically make dropping this a semver-major change…)

Yes, that's what I meant by not passing the test. Anyway, I totally don't mind changing it if that's what others want.

This comment has been minimized.

@trevnorris

trevnorris Jun 1, 2016

Contributor

I assume the isArrayBuffer check was only added in the first place to avoid crashes on the C++ side, but that doesn’t seem to be a concern anymore if everything’s done in JS.

Question is if we want to enforce an actual ArrayBuffer, or if users could passed a munged up object that looks like one. Personally I prefer to leave these in where possible b/c it makes code transitions between C++ and JS smoother (buffer has bounced all over the place in the last 3 years).

@addaleax

View changes

lib/buffer.js Outdated
@@ -351,7 +364,7 @@ Buffer.concat = function(list, length) {
length = length >>> 0;
}
var buffer = Buffer.allocUnsafe(length);
var buffer = allocate(length);

This comment has been minimized.

@addaleax

addaleax May 22, 2016

Member

Just to make sure, the s/Buffer.allocUnsafe/allocate/ changes here are completely unrelated to the rest of the changes, right?

This comment has been minimized.

@RReverser

RReverser May 22, 2016

Member

Yeah, just part of "performance improvement" by removing unnecessary indirections where safe to do so, but irrelevant to main optimizations.

This comment has been minimized.

@addaleax

addaleax May 31, 2016

Member

@RReverser Hmm, please take a look at #7051/#7079… doing s/Buffer.allocUnsafe/allocate/ would mean that the extra check for negative inputs would be removed.

This comment has been minimized.

@RReverser

RReverser May 31, 2016

Member

This is Buffer.concat - it either sums up lengths of existing Buffer list (throws error if they're not Buffers, and length of Buffer is always >=0) or performs length >>> 0 which makes it also always >= 0 (32-bit unsigned).

This comment has been minimized.

@addaleax

addaleax May 31, 2016

Member

Yeah, I think this one here can stay allocate(length).

return binding.createFromArrayBuffer(obj, byteOffset);
const maxLength = obj.byteLength - byteOffset;
if (maxLength <= 0)

This comment has been minimized.

@addaleax

addaleax May 22, 2016

Member

I think this should be <. I know this is taken directly from the C++ code, but I think 85ab4a5 actually introduced a regression here:

Node v5.9.1:

> new Buffer(new Uint8Array().buffer)
<Buffer >

Node v5.10.1:

> new Buffer(new Uint8Array().buffer)
RangeError: 'offset' is out of bounds

@jasnell?

This comment has been minimized.

@RReverser

RReverser May 22, 2016

Member

Happy to change if confirmed.

This comment has been minimized.

@trevnorris

trevnorris May 22, 2016

Contributor

Yup. Is regression. Nice catch.

This comment has been minimized.

@RReverser

RReverser May 22, 2016

Member

Should I change as part of this PR or as a separate one?

This comment has been minimized.

@Fishrock123

Fishrock123 May 22, 2016

Member

@RReverser Could always do a second commit here.

This comment has been minimized.

@RReverser

This comment has been minimized.

@RReverser

RReverser May 22, 2016

Member

Actually... Should I add a regression test for this?

This comment has been minimized.

@addaleax

addaleax May 22, 2016

Member

That would be great, yes.

This comment has been minimized.

@RReverser

This comment has been minimized.

@ChALkeR

ChALkeR Jun 3, 2016

Member

I'm not sure I got this correct — what happened here?

@trevnorris

View changes

lib/buffer.js Outdated
const ui8 = new Uint8Array(size);
Object.setPrototypeOf(ui8, Buffer.prototype);
return ui8;
return new FastBuffer(size);

This comment has been minimized.

@trevnorris

trevnorris May 22, 2016

Contributor

Can't verify myself atm. What is constructor.name of this?

This comment has been minimized.

@RReverser

RReverser May 22, 2016

Member

See the line above - https://github.com/nodejs/node/pull/6893/files#diff-196d056a936b6d2649721eb639e0442bR10 - I'm assigning constructor to Buffer, so the FastBuffer is invisible to the consumer.

This comment has been minimized.

@trevnorris

trevnorris May 24, 2016

Contributor

Ah yes. Missed that.

@@ -246,18 +247,30 @@ function fromArrayLike(obj) {
const length = obj.length;
const b = allocate(length);
for (var i = 0; i < length; i++)
b[i] = obj[i] & 255;
b[i] = obj[i];

This comment has been minimized.

@trevnorris

trevnorris May 22, 2016

Contributor

Why change this?

This comment has been minimized.

@RReverser

RReverser May 22, 2016

Member

Just another unnecessary indirection that slightly changes performance and, well, readability (we inherit from Uint8Array, which already performs this exact same normalization).

} else {
length >>>= 0;
if (length > maxLength)
throw new RangeError("'length' is out of bounds");

This comment has been minimized.

@trevnorris

trevnorris May 22, 2016

Contributor

style nit: ' not " for strings.

This comment has been minimized.

@RReverser

RReverser May 22, 2016

Member

Well, ESLint rules configured for the JS code seem to prohibit " except when otherwise leads to quote escaping inside of the string, so I tried to follow. Do you think '\'length\' is out of bounds' will look better? If so, happy to change, but believe that linter rules should be changed too to avoid confusion for future contributors.

This comment has been minimized.

@Fishrock123

Fishrock123 May 22, 2016

Member

I think we prefer

`'length' is out of bounds`

This comment has been minimized.

@RReverser

RReverser May 22, 2016

Member

ESLint complains against it too. I guess leaving as-is until the project styling rules are changed is the best option for now.

This comment has been minimized.

@trevnorris

trevnorris May 24, 2016

Contributor

I'm not sure why '"length" is out of bounds' or just 'length is out of bounds' would be unreasonable. Not like not having quotes would lead to confusion.

This comment has been minimized.

@RReverser

RReverser May 24, 2016

Member

Thought the same reasoning as below:

It probably would be considered a major change. Landing that in a separate PR sounds good to me.

(given that error message becomes slightly different, and this might be a breaking change)

This comment has been minimized.

@trevnorris

trevnorris May 24, 2016

Contributor

ah bugger. annoying that such minor changes would cause this, but you're right.

length >>>= 0;
return binding.createFromArrayBuffer(obj, byteOffset, length);
return new FastBuffer(obj, byteOffset, length);

This comment has been minimized.

@trevnorris

trevnorris May 22, 2016

Contributor

Now that you're directly extending Uint8Array, the checks above would only seem necessary if we're trying to preserve the error messages. Otherwise the Uint8Array constructor would catch it.

This comment has been minimized.

@RReverser

RReverser May 22, 2016

Member

Yup. Similar to the isArrayBuffer discussion above:

the expected TypeError is thrown when accessing byteLength below, although with a different message (which would technically make dropping this a semver-major change…)

Would it be a semver-major change if I remove those checks and amend messages in tests correspondingly? If so, should I submit that in a separate PR so that this one could land as a minor change?

This comment has been minimized.

@trevnorris

trevnorris May 24, 2016

Contributor

It probably would be considered a major change. Landing that in a separate PR sounds good to me.

This comment has been minimized.

@RReverser

RReverser May 24, 2016

Member

Cool, will do in a separate one then.

@RReverser

This comment has been minimized.

Member

RReverser commented May 22, 2016

Not directly related, but now that I'm trying to submit changes from my Windows machine (previous one was from Mac), I've found one error: .eslintrc reports every line as invalid due to linebreak-style: [2, "unix"] in .eslintrc, while Git by default matches OS native endlines and checks out with CRLF (unless .gitattributes specifies custom eol for all text files, and it doesn't in this repo).

What would be the best fix for this - a PR that removes .eslintrc rule against that or a PR that adds * text=auto eol=lf or [another option]?

@addaleax

This comment has been minimized.

Member

addaleax commented May 22, 2016

I think that discussion would best be taken to #6912 :)

@RReverser

This comment has been minimized.

Member

RReverser commented May 22, 2016

Oh cool, thanks! That's a new issue I haven't noticed yet :)

@RReverser

This comment has been minimized.

Member

RReverser commented May 22, 2016

Btw, can someone please explain what

function SlowBuffer(length) {
  if (+length != length)
    length = 0;

is for / supposed to do? Just tried to wrap my head around it, and wasn't sure whether the additional semantics on top of simple typeof length !== 'number' were intended or accidental.

@addaleax

This comment has been minimized.

Member

addaleax commented May 22, 2016

There was some discussion on that in #2635… though I can’t seem to find the advantage of using +length != length, either. It behaves differently for inputs like false or '42', and probably not even in a wanted way.

@addaleax

This comment has been minimized.

Member

addaleax commented May 22, 2016

Note that it only exists as part of a deprecated API anyway.

@RReverser

This comment has been minimized.

Member

RReverser commented May 22, 2016

It behaves differently for inputs like false or '42', and probably not even in a wanted way.

Exactly my thoughts.

Note that it only exists as part of a deprecated API anyway.

That's true, just looks pretty weird when trying to read / understand the code.

@addaleax

View changes

test/parallel/test-buffer-alloc.js Outdated
// Regression test
assert.doesNotThrow(() => {
new Buffer(new ArrayBuffer());

This comment has been minimized.

@addaleax

addaleax May 22, 2016

Member

This should preferably use Buffer.from instead of new Buffer

This comment has been minimized.

@RReverser
@addaleax

This comment has been minimized.

Member

addaleax commented May 22, 2016

I’d maybe separate the <= to < change, along with its regresseion test, out into its own commit that can be landed separately too.

LGTM either way.

@RReverser

This comment has been minimized.

Member

RReverser commented May 22, 2016

@addaleax Isn't 9d480d9 exactly that? (well, it also removes a comment but that's a non-functional change anyway).

@RReverser RReverser referenced this pull request May 22, 2016

Closed

buffer: Fix dataview-set benchmark. #6922

3 of 3 tasks complete
@addaleax

This comment has been minimized.

Member

addaleax commented May 22, 2016

@RReverser Kinda… you don’t have to move that if you don’t want to, but keep in mind that the commit history ideally still makes sense for someone looking at it in a few years, without having the context of this PR in mind. Happens more often than you think. :)

Also, it would be cool if you could re-format the commit message for that commit so that it adheres to the guidelines (i.e. it starts with buffer:, has an all-lowercase subject line, and is under < 72 columns)

@trevnorris

This comment has been minimized.

Contributor

trevnorris commented Jun 3, 2016

Hm. If this PR addresses the comment removal, leave it in its own commit. Yes it's minor, but if this needs to be reverted for some unforeseen reason don't want the comment coming back in.

As far as the regression, I vote we leave that to its own PR (since it'll require it's own regression test, etc.).

@addaleax

This comment has been minimized.

Member

addaleax commented Jun 3, 2016

@trevnorris Want to go ahead and land this then?

if (size <= 0)
return createBuffer(size);
if (fill !== undefined) {
if (size > 0 && fill !== undefined) {

This comment has been minimized.

@ChALkeR

ChALkeR Jun 3, 2016

Member

Does a separate check for 0 make sense here, i.e. size > 0 && fill !== undefined && fill !== 0?

Buffer.alloc(size, 0) is equivalent to Buffer.alloc(size), so just new FastBuffer(size) should work faster in that case.

This comment has been minimized.

@addaleax

addaleax Jun 3, 2016

Member

Hm, I think that would require benchmarking – createUnsafeBuffer() returns a slice from the pool, so I’d actually expect that to be faster than an extra typed array allocation.

This comment has been minimized.

@ChALkeR

ChALkeR Jun 3, 2016

Member

@addaleax It's not just createUnsafeBuffer, it's createUnsafeBuffer + fill(0).
I believe that has been discussed before, and allocation was proven to be faster — that's why simple Buffer.alloc(size) does not use the pool.

This comment has been minimized.

@addaleax

addaleax Jun 3, 2016

Member

@ChALkeR I know there’s the extra fill() in there. But if you say it’s faster, I believe that :)

This comment has been minimized.

@ChALkeR

ChALkeR Jun 3, 2016

Member

@addaleax Btw, nothing in this function uses slices from the pool. Perhaps it should?
Both new FastBuffer and createUnsafeBuffer just directly allocate a new instance.

This comment has been minimized.

@addaleax

addaleax Jun 3, 2016

Member

Oh, right. Maybe, but I’d leave that open for another PR, too, especially as it would introduce the subtle change that the return values of Buffer.alloc() would share their buffer property.

This comment has been minimized.

@trevnorris

trevnorris Jun 3, 2016

Contributor

i'm down for that change. The kernel can probably optimize calls to calloc() a hair better. Though not going to consider it a blocking change.

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Jun 3, 2016

LGTM

@trevnorris

This comment has been minimized.

Contributor

trevnorris commented Jun 3, 2016

@addaleax Going ahead w/ these sounds good to me.

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Jun 3, 2016

Curious: would

Buffer.alloc = function(size, fill, encoding) {
  assertSize(size);
  if (size > 0 && fill !== undefined && fill !== 0) {
    if (typeof encoding !== 'string')
      encoding = undefined;
    return allocate(size).fill(fill, encoding);
  }
  return new FastBuffer(size);
};

be faster for short buffers filled with some non-zero argument?

There are two changes here: && fill !== 0 (for Buffer.alloc(size, 0) opt), and createUnsafeBuffer to allocate change to use the pool for short buffers.

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Jun 3, 2016

On a second thought, we can do that in a separate PR, those are independent changes.
This PR LGTM.

@addaleax

This comment has been minimized.

Member

addaleax commented Jun 6, 2016

I’m going to land this later today if nobody beats me to it.

@RReverser

This comment has been minimized.

Member

RReverser commented Jun 6, 2016

On a second thought, we can do that in a separate PR, those are independent changes.
This PR LGTM.

Yes, thought about similar further optimizations, but they would be rather backward-incompatible and cases where they give any win are more rare, so decided not to change.

addaleax added a commit that referenced this pull request Jun 6, 2016

buffer: improve creation performance.
Improves performance of allocating unsafe buffers, creating buffers from
an existing ArrayBuffer and creating .slice(...) from existing Buffer by
avoiding deoptimizing change of prototype after Uint8Array allocation
in favor of ES6 native subclassing.

This is done through an internal ES6 class that extends Uint8Array and
is used for allocations, but the regular Buffer function is exposed, so
calling Buffer(...) with or without `new` continues to work as usual
and prototype chains are also preserved.

Performance wins for .slice are +120% (2.2x), and, consequently, for
unsafe allocations up to +95% (1.9x) for small buffers, and for safe
allocations (zero-filled) up to +30% (1.3x).

PR-URL: #6893
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
@addaleax

This comment has been minimized.

Member

addaleax commented Jun 6, 2016

Landed in 5292a13. Thanks for the contribution and for your patience with us!

@addaleax addaleax closed this Jun 6, 2016

@RReverser

This comment has been minimized.

Member

RReverser commented Jun 6, 2016

@addaleax Thank you! That was quite a trip, but totally fine as for the first PR to the project :)

@addaleax

This comment has been minimized.

Member

addaleax commented Jun 6, 2016

That was quite a trip

No arguing about that. 😄 If you like, you can also do PRs for some of the issues that popped up as side notes in the discussion here. If not, you don’t have to, of course.

  • That one obsolete comment mentioned here: #6893 (comment)
  • The regression for creating buffers from zero-length ArrayBuffers mentioned here: #6893 (comment)
  • The fill === 0 check/performance improvement mentioned here: #6893 (comment)

… if I’ve managed to get everything right. ;)

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Jun 6, 2016

@addaleax Also .alloc(size, fill) should probably use the pool since it fills manually either way — #6893 (comment).

@RReverser

This comment has been minimized.

Member

RReverser commented Jun 6, 2016

@addaleax

The regression for creating buffers from zero-length ArrayBuffers mentioned here: #6893 (comment)

Found that commit: 7454298

Should I just submit it as a new PR?

@addaleax

This comment has been minimized.

Member

addaleax commented Jun 6, 2016

@RReverser sounds good, yup :)

@RReverser RReverser referenced this pull request Jun 6, 2016

Closed

buffer: fix creating from zero-length ArrayBuffer #7176

3 of 3 tasks complete
@trevnorris

This comment has been minimized.

Contributor

trevnorris commented Jun 7, 2016

@ChALkeR It was deliberate to not have Buffer.alloc() use the pool, since it was introduced to force the user to safety, and allocating from the pool allows others to read your memory. For example:

var b;
while ((b = Buffer.allocUnsafe(1)).byteOffset > 0);
Buffer.from(b.buffer).fill(0);
setTimeout(() => {
  // See what else has been written to the buffer since
  console.log(b);
}, 3000);

Can collect more information by messing with Buffer.poolSize. The argument is that allowing those allocations to come from the pool undermines the secure aspect they're focused on.

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Jun 7, 2016

@trevnorris Ah, understood. I personally don't see how that is a problem, because .buffer properties are accessible only locally (i.e. not saved to the db, not transfered over network, etc), and we don't (and can't) gurantee any safety in presence of local malicious code.

But ok, let's keep it that way if there are concerns about that. Perhaps that should be documented as a small one-line comment in the source code?

@trevnorris

This comment has been minimized.

Contributor

trevnorris commented Jun 7, 2016

@ChALkeR That decision was simply my call when it was first implemented to make sure the PR would avoid additional scrutiny. If everyone's alright with using the pool then I won't stand in the way.

@rvagg rvagg referenced this pull request Jun 8, 2016

Closed

governance: add new collaborators XIV #7197

5 of 5 tasks complete
@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Jun 8, 2016

@trevnorris On a second though, I think that you are correct here and that we should keep that as it is now. There could be various code errors on user side which could potentially cause issues if the code somehow uses the .buffer property.

Also, the current behaviour is documented, and changing that would be a semver-major.

So let's not change that =).

@evanlucas

This comment has been minimized.

Member

evanlucas commented Jun 16, 2016

This depends on #7082 and #7093, both of which have been marked dont-land-on-v6.x. @RReverser interested in opening a backport PR against the v6.x branch?

@RReverser

This comment has been minimized.

Member

RReverser commented Jun 16, 2016

#7176 (comment) same question here

RReverser added a commit that referenced this pull request Jun 25, 2016

buffer: improve creation performance.
Improves performance of allocating unsafe buffers, creating buffers from
an existing ArrayBuffer and creating .slice(...) from existing Buffer by
avoiding deoptimizing change of prototype after Uint8Array allocation
in favor of ES6 native subclassing.

This is done through an internal ES6 class that extends Uint8Array and
is used for allocations, but the regular Buffer function is exposed, so
calling Buffer(...) with or without `new` continues to work as usual
and prototype chains are also preserved.

Performance wins for .slice are +120% (2.2x), and, consequently, for
unsafe allocations up to +95% (1.9x) for small buffers, and for safe
allocations (zero-filled) up to +30% (1.3x).

PR-URL: #7349
Ref: #6893
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment