feature: support initializing Buffer from an ArrayBuffer

[feross]

I propose we make the Buffer constructor also accept ArrayBuffer.

Currently this is what happens:

var u = new Uint8Array([1, 2, 3, 4]);
var ab = u.buffer;
var b = new Buffer(ab);
console.log(b); // <Buffer >

This is what should happen:

console.log(b); // <Buffer 01 02 03 04>

When writing isomorphic code (i.e. code that runs on the server and in the browser), it's often the case that you'll get an ArrayBuffer from a DOM API (xhr, websockets, webrtc, etc.) and need to convert it to a Buffer to work with modules in the npm ecosystem. Users often expect that new Buffer(arraybuffer) will work and they open issues when it doesn't.

We have the buffer npm module which gives us the same Buffer API in the browser (and is used by browserify), however it tracks the node.js/io.js buffer exactly, so we can't add support for new Buffer(arraybuffer) unless core does too.

I know the Buffer constructor already takes a million different argument types, so it couldn't hurt to add one more, right? Curious to see what the community thinks about this. If there's interest, I can send a PR.

[chrisdickinson]

Curious: would this copy the memory represented by the ArrayBuffer, or act as a view onto it?

[vkurchatkin]

@chrisdickinson It should behave the same way as new Buffer(buffer) i.e. copy, but the latter is impossible anyway

[cjihrig]

I think it makes sense to make the binary data APIs as compatible as possible.

[trevnorris]

Reason this is a pain is because you can't access the void* of the data contained in the ArrayBuffer without needing to make it External. Meaning you have to take control of the life span of the memory. Which completely defeats GC optimizations in V8.

I've already gone down this road, and it's a bigger pain then it seems. Just a warning.

[trevnorris]

@feross The reason you're seeing that result is because internally v8::Object::HasIndexedPropertiesInExternalArrayData() returns true, but v8::Object::GetIndexedPropertiesExternalArrayData() returns NULL because the size of the allocation is small enough that it happens on the V8 heap. If you tried it w/ a larger ArrayBuffer you'd probably see different results.

[chrisdickinson]

@vkurchatkin I bring this up since the buffers in browserify are implemented on top of typed arrays. The ideal behavior (in browser, at least) would be to present the Buffer as a view of the given ArrayBuffer, I imagine. A copy operation, OTOH, would be implementable in JS, but much slower for the desired use case. For example, if an XHR hands you an ArrayBuffer representing 8mb it would be nice to be able to wrap that up as a Buffer without having to copy the entire contents -- and if that's the desired behavior in browser, it would be best if new Buffer(arrayBuffer) had the same semantics in Node.

[bnoordhuis]

@chrisdickinson Reference ("view") semantics probably won't be much faster. For the Buffer object to get a pointer to the ArrayBuffer's memory, it needs to externalize the ArrayBuffer. That means copying its contents from the JS heap to the C heap; besides being slow, it also makes it more likely to hit out-of-memory errors, because it needs 2 * arraybuffer.byteLength + slop memory.

An approach that doesn't need additional memory is to conjure up a Buffer instance that intercepts numeric loads and stores and looks them up in the ArrayBuffer. That's probably at least a hundred times slower, though.

[feross]

@chrisdickinson Agreed. Presenting the Buffer as a view on the ArrayBuffer makes the most sense in the browser context. But it would diverge from the way the Buffer constructor handles all other argument types, i.e. copying them.

For the record, to convert an ArrayBuffer or typed array to a Buffer without a copy in the browser, you can use typedarray-to-buffer.

[phaux]

Wouldn't it be possible to completely obsolete Buffers and encourage moving onto ArrayBuffers? Don't we have Buffers only because there was no ArrayBuffers when Node was first released?

[trevnorris]

@phaux No. Typed Arrays can't be extended. So it would be impossible to have an API where the data can both be accessed by index, and have custom methods on it. Also ArrayBuffer requires a zero fill on the data, which on the server isn't necessary for most cases and comes at a performance hit. Plus a few others. Array Buffers are great for the client, but not what's best for a server application.

[feross]

That only handles typed array views like Uint8Array, but it doesn't handle ArrayBuffer. Please re-open.

[trevnorris]

@feross Do you want to allow setting byte range's, or just use the entire ArrayBuffer?

[feross]

@trevnorris Using the entire ArrayBuffer is probably fine since you can just wrap the ArrayBuffer in a Uint8Array to get a slice of it. The Uint8Array constructor takes Uint8Array(arraybuffer [, byteOffset [, length]]). But we could make the API nicer by supporting these same arguments in the Buffer constructor.

The real question: do you want to do a copy or just make the buffer point to the same memory? All the other argument types to Buffer do a copy, so there's an argument for just doing that. On the other hand, the typed array constructors (e.g. Uint8Array, etc.) treat an ArrayBuffer argument differently, and point to the same memory. The API is Uint8Array(arraybuffer [, byteOffset [, length]]).

[vkurchatkin]

The real question: do you want to do a copy or just make the buffer point to the same memory

I vote for copy. We could have alternative API for views (Buffer.from for example)

[trevnorris]

@vkurchatkin That also sounds the most reasonable from the existing API, which will make a copy of a buffer instance if passed.

[feross]

@trevnorris Uint8Arrays also do a copy if you pass in another Uint8Array instance.

I think it would be nice if the Buffer constructor kept parity with typed arrays as much as possible. We pretty much have that right now, but if we make ArrayBuffer do a copy then we'll diverge.

See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray#Syntax

[trevnorris]

@feross That also makes sense being that Buffer will basically become an extension of Uint8Array.

I honestly don't care either way. I have a patch ready, and the difference between copy or use is 2 lines of code. Implementing the byteOffset and length parameters is pretty trivial, so I'll throw that in as well.

Just let me know what you all want.

[Fishrock123]

Whatever is the least surprising probably, so maybe like the builtins unless they do things in not-reasonable ways?

[trevnorris]

Then I'd say using the ArrayBuffer. Since Buffer is essentially just an extension of Uint8Array.

[trevnorris]

Fixed by 197ba00.

[Tiagoao99]

I propose we make the Buffer constructor also accept ArrayBuffer.

Currently this is what happens:

var u = new Uint8Array([1, 2, 3, 4]);
var ab = u.buffer;
var b = new Buffer(ab);
console.log(b); // <Buffer >

This is what should happen:

console.log(b); // <Buffer 01 02 03 04>

When writing isomorphic code (i.e. code that runs on the server and in the browser), it's often the case that you'll get an ArrayBuffer from a DOM API (xhr, websockets, webrtc, etc.) and need to convert it to a Buffer to work with modules in the npm ecosystem. Users often expect that new Buffer(arraybuffer) will work and they open issues when it doesn't.

We have the buffer npm module which gives us the same Buffer API in the browser (and is used by browserify), however it tracks the node.js/io.js buffer exactly, so we can't add support for new Buffer(arraybuffer) unless core does too.

I know the Buffer constructor already takes a million different argument types, so it couldn't hurt to add one more, right? Curious to see what the community thinks about this. If there's interest, I can send a PR.