Buffer Use Uint8Array by trevnorris · Pull Request #1825 · nodejs/node · GitHub

This is still partially incomplete. All tests are passing, but some parts still need to be hardened. The native API needs to return MaybeLocal<>, and CreateFromString() needs to check the return value to make sure the string could be written.

All existing tests are passing. A good chunk of the JS changes are whitespace related. For the conditional logic to allow --use-old-buffer flag.

R=@bnoordhuis

Allowing the name to be passed to the ARGS_THIS macro will make it easier to share code with the Uint8Array implementation.

Because we can't resize these Buffers as we could previously, the complete string write is done on the native side so the memory can be shrunk for base64 encoding before it is passed to the new buffer.

Buffers now have a maximum size of Smi::kMaxLength, as defined by V8.
Which is ~2 GB on 64 bit and ~1 GB on 32 bit.

What happened to the arbitrary sized buffers you talked about on Twitter?

@targos ArrayBuffer can be arbitrary sized, but Typed Arrays have a maximum index value. My initial code was all C++, so I read the data into an ArrayBuffer instance. Then was performing operations on that data using the native Buffer methods. Was later that night when I realized that creating a new Uint8Array from an ArrayBuffer that large was throwing. After some investigation saw that V8 sets a limitation (https://github.com/v8/v8-git-mirror/blob/4.3.66/src/api.cc#L6453-L6456). This is something I hope to get fixed in the future so Typed Arrays can also have arbitrarily long indices.

Though I was also thinking that it may be useful to allow creating new Buffer instances from slices of an ArrayBuffer. Will look into that after this PR is finished.

Link to V8 issue I created to hopefully one day allow a much higher upper limit on Typed Array indices: https://code.google.com/p/v8/issues/detail?id=4153

Thanks for the explanation.
FWIW I starred your issue.

Might not need any of the custom iterator implementation for the new version (i.e. can hide all this behind an if)

Already do, and you're right. It doesn't. :)

Ah, just scrolled past that part, sorry!

@targos Thanks :)

For anyone curious about benchmarks, here are a few results I compiled from the benchmarks folder. Left column is old implementation. Right column is with this PR:

buffer-slice.js type=fast n=1024: 6646.1  2570.8
buffer-slice.js type=slow n=1024: 6711.2  2617.4

buffer-creation.js type=fast len=10 n=1024:   5103.1  2498.6
buffer-creation.js type=fast len=1024 n=1024: 2648.3  2054.9
buffer-creation.js type=slow len=10 n=1024:   2190.0  2067.8
buffer-creation.js type=slow len=1024 n=1024: 1294.9  1677.7

http-simple type=bytes length=1024:   21588.6  18053.3
http-simple type=bytes length=10240:  8601.9   8439.7

I'll post more once I've done more precise testing.

In IRC you mentioned that there is no more .parent, instead you use .buffer. Maybe we should alias it (with a getter if nothing else)?

In general what would the changelog look like here? Especially in terms of back-compat breakers? I imagine most of them are on the C++ side but those are worth putting in the changelog too.

@domenic Most the breakage is on the C++ side. The only JS changes that break, that I'm aware of, are removal of .parent and .offset (which are now .buffer and .byteOffset).

On the side, using .buffer wasn't a personal choice. That's what property the ArrayBuffer is set to for a Typed Array.

Making each one a getter shouldn't be difficult, if we'd like to maintain full backwards compatibility that way.

Yeah, it seems relatively worthwhile to add aliasing-getters for .parent and .offset. A little crufty, but worthwhile IMO.

Suggestion: instead of having if statements everywhere, maybe move the old and new code to two files buffer_new.js and buffer_old.js in lib/internal? Then you just require() the right one here and re-export it.

Ooh. I like that.

is new Buffer(size) initially filled with 0 now ?

Maybe (Buffer.poolSize >>> 1)? Probably doesn't matter much performance-wise.

Did this before I realized that just because ArrayBuffers can be arbitrarily big, typed arrays can't. So it was "technically" possible to set the pool size greater than a bitwise operation could handle. But that's not actually the case. So I'll change that back.

With V8 4.4 removing the external array data API currently used by Buffer, the new implementation uses the Uint8Array to back Buffer. Buffers now have a maximum size of Smi::kMaxLength, as defined by V8. Which is ~2 GB on 64 bit and ~1 GB on 32 bit. The flag --use-old-buffer allows using the old Buffer implementation. This flag will be removed once V8 4.4 has landed. The two JS Buffer implementations have been split into two files for simplicity. Use getter to return expected .parent/.offset values for backwards compatibility. PR-URL: #1825 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Passing a FreeCallback to Buffer::New() now uses externalized ArrayBuffer's. PR-URL: #1825 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Use the new Maybe<T> syntax for v8::Object::SetPrototype(). PR-URL: #1825 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Instead of aborting in case of internal failure, return an empty Local<Object>. Using the MaybeLocal<T> API, users must check their return values. PR-URL: #1825 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Address comments and deprecations left in source files. These changes include: * Remove the deprecated API. * Change Buffer::New() that did a copy of the data to Buffer::Copy() * Change Buffer::Use() to Buffer::New() PR-URL: #1825 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

New Buffer implementation allows greater than kMaxLength to be created. So instead check if the passed value is a valid Smi. PR-URL: #1825 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Allowing the name to be passed to the ARGS_THIS macro will make it easier to share code with the Uint8Array implementation. PR-URL: #1825 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>