691446 - Give buffer ownership to a TypedArray

Status: NEW

(Core :: JavaScript Engine, enhancement)

Description

[Franck]

I need to create a new TypedArray from a buffer already allocated with JS_malloc.
The aim is to avoid useless buffer copy.

Comment 1

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Buffer stealing API.

Needs tests if we think this is a good idea.

Comment 2

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: WIP: buffer stealing API.

qref

Comment 3

[Chris Leary [:cdleary] (not checking bugmail)]

Franck, this is definitely an implementation-detail API that would only exist for performance reasons and could totally break in the future. (In retrospect, I should have made it a JS_FRIEND_API.)

Our internal implementation for the ArrayBuffer's backing store is a js::Value-modulo sized array buffer where the uint32 at the front of the buffer is the length of the array (per the comments on the API function). If you have to move all the values in order to stick the uint32 in front, you won't really be avoiding much overhead. Does this work for your use case?

Comment 4

[Jeff Walden [:Waldo]]

(In reply to Franck from comment #0)
> I need to create a new TypedArray from a buffer already allocated with
> JS_malloc.
> The aim is to avoid useless buffer copy.

Why not do it the other way around?

"I need to be able to access the buffer from an already allocated TypedArray."

That can be much more future-proof, I think.  If we don't have this API (I seem to recall a bug on it awhile back), we should add it.

Comment 5

[Franck]

My use case is quite simple:
Among other, I have a socket.Read() function that reads an undefined amount of data in a byte buffer (JS_malloc/JS_realloc).
This buffer is wrapped with a Blob JSObject and returned from socket.Read()
Then the user can get a string version or a TypedArray version (or whatever version) of the buffer through the Blob object (after that, the Blob object become invalidated).

1/ The first proposition from Chris Leary means that the Socket.Read() will need to reserve a uint32 at the front of its buffer even if the string version will be used later.

2/ The proposition from Jeff Walden means that the TypedArray needs to be created, even if it will not be used.

Maybe, and If the TypedArray allow to be resized (like realloc), I will be able to replace my Blob with a TypeArray,
and create an new ExternalString that points to the begining of TypeArray buffer ?

Comment 6

[Jeff Walden [:Waldo]]

Typed arrays can't be resized.  You create the typed array, it has a length, it has it forever.  I don't think we should change this.  We'd be different from everyone else, and the problems from doing so would be all our own.

I've thought about the reallocing problem and typed arrays before, in the context of the web server used in Mochitests.  It's not clear to me what the best solution is, that avoids buffer copying, without either forcing certain storage requirements under the hood or requiring that the code that does the read have the typed array and store directly into it.

Digging a little deeper into your socket.Read(): what's the underlying interface you're wrapping?  available() plus read(n, buffer with size at least n)?  Or something else?  I'm wondering if we could possibly avoid the realloc somehow, or come up with some other clever trick here.

Comment 7

[Franck]

I use NSPR that has PR_Available(). However my function socket.Read has an optional [amount] argument. When not given, the function tries to read everything until the socket is closed.
Furthermore, in some situations, it is not possible to know the length in advance (HTTP1.0 connection: close, recording an audio or video stream, data serialization, data encoding, …)
Usually, to manage these situations, I use realloc() or I allocate a series of small buffers that I join into a new buffer once the whole data is read.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 564321 [details] [diff] [review]
WIP: buffer stealing API.

Review of attachment 564321 [details] [diff] [review]:
-----------------------------------------------------------------

This patch is fine modulo one nit, but given that this isn't quite useful for anyone yet, let's hold off on committing it.

::: js/src/jsapi.cpp
@@ +4625,5 @@
>  
> +JS_PUBLIC_API(JSObject *)
> +JS_CreateArrayBuffer(JSContext *cx, char *bytes, size_t size)
> +{
> +    JS_ASSERT((size & uint32(-1)) == 0);

This isn't quite what you want.

JS_ASSERT(size == uint32(size)) seems like a simpler way of doing what you want.

Comment 9

[Josh Matthews [:jdm]]

> This patch is fine modulo one nit, but given that this isn't quite useful
> for anyone yet, let's hold off on committing it.

According to the readme at https://github.com/mbebenita/Broadway, this patch does have some use to the wider world.

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

Oh my, the readme does say it's dependent on this patch. I suppose we should land it. I'll sr? dmandelin for the API change.

Comment 11

[David Mandelin [:dmandelin]]

Comment on attachment 564321 [details] [diff] [review]
WIP: buffer stealing API.

I usually don't sr patches until they've been r+d. The API seems OK...., but is pretty sharp-edged. I'd like to see a concise rationale for (a) why a new API is necessary at all, and (b) why this API in particular.

Comment 12

[Till Schneidereit [:till]]

Mass-reassigning cdleary's bugs to default. He won't work on any of them, anymore. I guess, at least.

@cdleary: shout if you take issue with this.