Closed Bug 720949 Opened 13 years ago Closed 12 years ago

Need API for "transferring" ArrayBuffer data between runtimes (via shared memory)

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla18

People

(Reporter: bent.mozilla, Assigned: sfink)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Add JSAPI for transferring ArrayBuffer contents
(deleted), patch
luke
: review+
Details | Diff | Splinter Review 
Web Workers are about to standardize a transfer extension to postMessage that will allow quick passing of ArrayBuffer data to/from workers. As far as I can tell there is no way to create an array buffer that is backed by non-runtime-specific heap memory, and we will need something like that in order to make this fast.
And while we're at it, it'd be great to be able to construct an ArrayBuffer which is backed by a malloc'ed buffer. This is needed by for example XMLHttpRequest which streams data into a buffer, and once all data has been downloaded wants to construct an ArrayBuffer containing that data. Since the XHR doesn't know the size of the buffer until the download is done, it can't create an ArrayBuffer instance and download the data directly into that.

So if we had a way to back an ArrayBuffer by a malloc'ed buffer, it seems like the use cases in comment 0 as well as the XHR one could be solved?
What does the spec say happens to the original object?  Does it become invalidated in some way?
It says it's neutered.  (Term of art, honest!)

That said, based on my dim memory of skimming the spec and happening to see this a week or so ago, it may be ECMAScript-semantically ill-conceived.  Neutering an ArrayBuffer causes the byteLength of that buffer to become 0.  But byteLength is a non-writable property, and for that to have any teeth, it must also be non-configurable.  If that's the case, ECMAScript forbids the observable behavior of an immutable property changing its value or disappearing.  Making byteLength a getter without a setter might be one fix for that.

The individual indexed properties of the ArrayBuffer are another concern, at least if someone attempted to make one or more such properties non-configurable.  But that has interactions with how WebIDL says the descriptions in the spec map to concrete things, and I don't know that well enough to say with much confidence that there is or is not a concern.
Oddly enough, just a few days ago I added an internal API that does roughly that in order to implement ArrayBuffer.slice:

struct JS_FRIEND_API(ArrayBuffer) {
    static JSObject *create(JSContext *cx, int32_t nbytes, uint8_t *contents = NULL);

Does it get you what you want if I just add a new variant of JS_NewArrayBuffer that takes a |contents| parameter?

My only question is about the original buffer: (a) it has to be a standard malloc'd buffer (cx->malloc_ is what the basic version uses, but I think the only thing special about that is some GC memory accounting) so that the finalizer does the right thing, and (b) it had better not get modified later via the original pointer. Are both of those going to work for you?
What does "it had better not get modified later via the original pointer" mean? Surely we're allowed to modify the contents of the malloc'ed buffer, right?

Not being able to modify the pointer sounds ok.

We do still need two more things though:

1. The ability to "truncate" an ArrayBuffer. I.e. set it's size to 0. This is
   what happens to an ArrayBuffer when it's "transferred" to a Worker. We need to
   be able to do this on all types of ArrayBuffers, including ones that use an
   internal buffer.
2. For ArrayBuffers which are backed by a malloc'ed buffer, we need to be able to
   take ownership of the malloced buffer as we truncate it.

This way when a malloced arraybuffer is transferred, we can transfer the data to the worker without doing any memory copying. Ownership of the malloc'ed buffer is simply transferred to a newly constructed ArrayBuffer which lives inside the worker. If someone transfers a non-malloc'ed ArrayBuffer we'll have to copy the data, but we can still maintain the same behavior outwards.
(In reply to Jonas Sicking (:sicking) On leave waiting for visa from comment #5)
> What does "it had better not get modified later via the original pointer"
> mean? Surely we're allowed to modify the contents of the malloc'ed buffer,
> right?

Well, thinking about it more, it seems that in a single-threaded setup it would fine to modify the contents, because anything you could do that way you could do by creating a Uint8Array view and setting bytes. But if it's being passed from thread T1 to T2, then T1 shouldn't touch the buffer until T2 stops.

> We do still need two more things though:
> 
> 1. The ability to "truncate" an ArrayBuffer. I.e. set it's size to 0. This is
>    what happens to an ArrayBuffer when it's "transferred" to a Worker. We
> need to
>    be able to do this on all types of ArrayBuffers, including ones that use
> an
>    internal buffer.
> 2. For ArrayBuffers which are backed by a malloc'ed buffer, we need to be
> able to
>    take ownership of the malloced buffer as we truncate it.
> 
> This way when a malloced arraybuffer is transferred, we can transfer the
> data to the worker without doing any memory copying. Ownership of the
> malloc'ed buffer is simply transferred to a newly constructed ArrayBuffer
> which lives inside the worker. If someone transfers a non-malloc'ed
> ArrayBuffer we'll have to copy the data, but we can still maintain the same
> behavior outwards.

How about:

/*
 * Create a new array buffer with the given contents, which must be valid
 * memory of at least |nbytes|. The new array buffer takes ownership of
 * the contents array. After calling this function, do not free |contents| or
 * use |contents| from another thread.
 */
JSObject *
JS_NewArrayBufferWithContents(JSContext *cx, uint32_t nbytes, uint8_t *contents);

/*
 * Steal the contents of the given array buffer. The array buffer has its
 * length set to 0 and its contents array cleared. The contents array and
 * its length are returned via |nbytes| and |contents|. The caller takes
 * ownership of |contents| and must free it or transfer ownership when done 
 * using it.
 */
void
JS_StealArrayBufferContents(JSContext *cx, JSObject *obj, uint8_t *nbytes, uint8_t **contents);
(In reply to David Mandelin from comment #6)
> (In reply to Jonas Sicking (:sicking) On leave waiting for visa from comment
> #5)
> > What does "it had better not get modified later via the original pointer"
> > mean? Surely we're allowed to modify the contents of the malloc'ed buffer,
> > right?
> 
> Well, thinking about it more, it seems that in a single-threaded setup it
> would fine to modify the contents, because anything you could do that way
> you could do by creating a Uint8Array view and setting bytes. But if it's
> being passed from thread T1 to T2, then T1 shouldn't touch the buffer until
> T2 stops.

Yes. In practice I don't think we'll be touching the contents of the buffer ever. But if we do, we'll definitely only do it on the thread where we have an ArrayBuffer owning the malloc'ed buffer.

> JSObject *
> JS_NewArrayBufferWithContents(JSContext *cx, uint32_t nbytes, uint8_t
> *contents);
...
> void
> JS_StealArrayBufferContents(JSContext *cx, JSObject *obj, uint8_t *nbytes,
> uint8_t **contents);

I assume that JS_StealArrayBufferContents only works on ArrayBuffers which hold a malloc'ed buffer?

If so, this doesn't yet completely solve requirement 1 from comment 5. I.e. we need the ability to truncate a non-malloc'ed ArrayBuffer as well as check if an ArrayBuffer is backed by a malloc'ed buffer. Here's the code that I'm imagining we'll write:


On thread 1:
JSObject* arraybuffer = ...;
uint32 size;
uint8_t* contents = nsnull;
if (JS_ArrayBufferHasStealableContents(cx, arraybuffer)) {        // Still missing
  JS_StealArrayBufferContents(cx, arraybuffer, &size, &contents);
}
else {
  size = JS_GetArrayBufferByteLength(arraybuffer);
  contents = malloc(size);
  <OOM check contents>;
  memcpy(contents, JS_GetArrayBufferData(arraybuffer), size);
  JS_TruncateArrayBuffer(cx, arraybuffer);                        // Still missing
}
sendToOtherThread(contents, size);

On thread 2:
JSObject* newArraybuffer = JS_NewArrayBufferWithContents(cx, aSize, aContents);
(In reply to Jonas Sicking (:sicking) from comment #7)
> (In reply to David Mandelin from comment #6)
> > (In reply to Jonas Sicking (:sicking) On leave waiting for visa from comment
> > #5)
> > JSObject *
> > JS_NewArrayBufferWithContents(JSContext *cx, uint32_t nbytes, uint8_t
> > *contents);
> ...
> > void
> > JS_StealArrayBufferContents(JSContext *cx, JSObject *obj, uint8_t *nbytes,
> > uint8_t **contents);
> 
> I assume that JS_StealArrayBufferContents only works on ArrayBuffers which
> hold a malloc'ed buffer?

IIUC, all ArrayBuffers have their internal elements array allocated via malloc (whether created internally or supplied to the creation function), so once the ArrayBuffer is created there is no distinction and they are all stealable.

If that for some reason turned out not to be true, then I would probably want to keep the same API as above, but let it alloc-copy-and-truncate internally (i.e., put the code you gave into the API implementation).

Does that sound right?
Assignee: general → sphink
(In reply to David Mandelin from comment #8)
> IIUC, all ArrayBuffers have their internal elements array allocated via
> malloc (whether created internally or supplied to the creation function), so
> once the ArrayBuffer is created there is no distinction and they are all
> stealable.

Not quite. ArrayBuffers will put their data directly into the fixed slots area if there's enough space. Which isn't a big deal; those will be tiny buffers, so we can easily malloc a fresh buffer for the steal operation.

> If that for some reason turned out not to be true, then I would probably
> want to keep the same API as above, but let it alloc-copy-and-truncate
> internally (i.e., put the code you gave into the API implementation).
> 
> Does that sound right?

ArrayBuffers currently malloc a single buffer, containing a fixed-size header region and then the actual data. So the above API wouldn't work out quite right; if Steal() returned the allocated pointer, then we'd need a new Create() api that knew that the header was already included. (Or if Steal() returned the pointer to the actual data, then it wouldn't be free-able.)
I think Dave's proposal is that for the case when the ArrayBuffer uses slots or a header+buffer allocation, the Steal function would internally call malloc(), copy all the data over to the new buffer and then set the ArrayBuffer's internal size to 0.

If we're using slots then setting the size to 0 wouldn't actually free any memory, but we still need to do it in order to keep the API consistent.
(In reply to Jonas Sicking (:sicking) from comment #11)
> I think Dave's proposal is that for the case when the ArrayBuffer uses slots
> or a header+buffer allocation, the Steal function would internally call
> malloc(), copy all the data over to the new buffer and then set the
> ArrayBuffer's internal size to 0.

Currently, that covers 100% of possible cases. So unless JS_NewArrayBufferWithContents() constructs a third, entirely new memory layout for ArrayBuffers, this will not help with the motivation given in comment 0, which is that this is supposed to be a fast path. (Or at least, you'll pay for alloc + memcpy. I guess it's still better than iterating over it and copying element by element via JS.)

> If we're using slots then setting the size to 0 wouldn't actually free any
> memory, but we still need to do it in order to keep the API consistent.

Understood.

I see a couple of options for how to proceed:

1. Implement the above just to have the API in place and make it fast later. This might make more sense than it sounds like, given that there is work in progress to provide separate storage in all JSObjects for indexed vs non-indexed properties, which might make the optimization easier (or at least, more maintainable.)

2. Add a third memory layout where the element header is divorced from the elements' data. Right now the elements are handled by standard JSObject code, which assumes that you can get to the element header by calling elements() and subtracting sizeof(ObjectElements). This is all cleanly abstracted in JSObject, but it seems like it would add a bit check and branch on what I assume is a hot path. It might even require updates to JIT code generation; I'm not sure.

3. Same as #2, but eliminate the current header+elements storage layout for ArrayBuffers, just to minimize complexity. Or for everything.

4. Modify the proposed API to instead of returning <size,contents pointer>, returning enough information to reconstruct a new ArrayBuffer without changing the memory layout. (So it'd probably just return the pointer to the malloc'd buffer, and it would use the contents of the ObjectElements header containined within it to figure out the size.) This handles the fast passing of ArrayBuffers via postMessage, but does *not* help the use case of having a just-the-data pointer and constructing an ArrayBuffer from it.

5. To handle the latter, would it be possible to have an API for returning sizeof(ObjectElements) so the caller could reserve that much extra space at the beginning of its buffer? Seems kind of ugly, I admit.

6. Allow postMessage or the structured cloning stuff or whatever it is to send ArrayBuffer objects around, and do the ownership transfer there. (So the ArrayBuffer JSObject would get a brain transplant into a newly-constructed JSObject in the destination compartment, the malloced memory would be reused with no changes, and the source ArrayBuffer JSObject would get neutered.)

It's very possible that I'm totally off base here, and I'm just reading the code wrong.

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #3)
> That said, based on my dim memory of skimming the spec and happening to see
> this a week or so ago, it may be ECMAScript-semantically ill-conceived. 
> Neutering an ArrayBuffer causes the byteLength of that buffer to become 0. 
> But byteLength is a non-writable property, and for that to have any teeth,
> it must also be non-configurable.  If that's the case, ECMAScript forbids
> the observable behavior of an immutable property changing its value or
> disappearing.  Making byteLength a getter without a setter might be one fix
> for that.

Maybe this is the same as what you're saying, but is it legal to make it non-configurable but writable, and have the setter throw on any modification? (Or only allow setting to zero, which would permanently neuter a buffer, for people who want the joys of manual memory management.) So it would be "writable so that it is allowed to change", but not "writable meaning you can actually modify the value." Perhaps that's what having "a getter without a setter" means?
In your XHR case, where is that data buffer coming from? Is some size malloced, then when the response turns out to be larger you realloc?

What about:

JSBool
JS_AllocateArrayBufferContents(JSContext *cx, uint32_t payloadBytes, uint8_t **contents, uint32_t *headerSize);

It would return a free-able, realloc-able pointer in *contents that points to a memory buffer of size payloadBytes + *headerSize, but the first *headerSize bytes are off-limits.

Or as an alternative, 

uint32_t JS_ArrayBufferHeaderSize(JSContext *cx);

that returned the header size, and it's up to you to allocate the memory?

You would then pass your contents buffer into

JS_NewArrayBufferWithContents(JSContext *cx, uint32_t nbytes, uint8_t *contents);

which is NOT the same as what dmandelin had, because |contents| could not be an arbitrary pointer to data. It'd have to be something returned from JS_AllocateArrayBufferContents (or malloced with JS_ArrayBufferHeaderSize bytes.)

This is kind of dirty, in that it exposes internal details of JSObject memory management that I could easily see changing. I suppose I could add extra apis JS_ReallocateArrayBufferContents and JS_FreeArrayBufferContents, and then remove the headerSize parameter from JS_AllocateArrayBufferContents. That's more future-proof, but a bit clunkier.
For V8Monkey to be able to fully support V8's API, something along the lines of dmandelin's JS_NewArrayBufferWithContents is required.

V8 has the function Object::SetIndexedPropertiesToExternalArrayData[1] which, IIUC, can't be implemented using sfink's JS_NewArrayBufferWithContents.
(In reply to Steve Fink [:sfink] from comment #13)
> In your XHR case, where is that data buffer coming from? Is some size
> malloced, then when the response turns out to be larger you realloc?

Actually, right now we just append the data to the end of an XPCOM string (nsCString) which means that we can't reuse the buffer anyway.

A better implementation would likely be to do a malloc for each individual buffer we get from the network code and just keep an array of dis-joint buffers until we know we have all data. If we did that then we wouldn't need to be able to create ArrayBuffers whose data is free-able.


That would simplify the requirements to:

1. A function which given an ArrayBuffer can return an opaque object which contains the ArrayBuffer's data. This function may or may not need to return the length separately. This function would always return such a buffer even if it internally uses slots. The function would also always truncate the buffer (which may or may not free any data).

2. A function which constructs an ArrayBuffer using a said opaque object (and possibly a separate length).

3. A function which can destroy an opaque object. Needed if we end up killing the worker before we get a chance to create the ArrayBuffer. Ideally this function is simply 'free'.


So the API in comment 6 would work, but with "contents" having the semantic of "opaque object you shouldn't touch, allocated with malloc". And we could possibly drop the "nbytes" arguments if we feel that we don't need it.
(In reply to Jonas Sicking (:sicking) from comment #16)
> 1. A function which given an ArrayBuffer can return an opaque object which
> contains the ArrayBuffer's data. This function may or may not need to return
> the length separately. This function would always return such a buffer even
> if it internally uses slots. The function would also always truncate the
> buffer (which may or may not free any data).
> 
> 2. A function which constructs an ArrayBuffer using a said opaque object
> (and possibly a separate length).
> 
> 3. A function which can destroy an opaque object. Needed if we end up
> killing the worker before we get a chance to create the ArrayBuffer. Ideally
> this function is simply 'free'.
> 
> So the API in comment 6 would work, but with "contents" having the semantic
> of "opaque object you shouldn't touch, allocated with malloc". And we could
> possibly drop the "nbytes" arguments if we feel that we don't need it.

Yep, this is option #4 in my list. It seems reasonable.

(In reply to Till Schneidereit [:till] from comment #14)
> For V8Monkey to be able to fully support V8's API, something along the lines
> of dmandelin's JS_NewArrayBufferWithContents is required.
>
> V8 has the function Object::SetIndexedPropertiesToExternalArrayData[1]
> which, IIUC, can't be implemented using sfink's
> JS_NewArrayBufferWithContents.

Yes. I think this is really a separate requirement, though, and should be considered after Waldo's property/element split. (That will bring us more in alignment with the V8 API in general, btw.)

Please file a new bug. Specifically, it sounds like you need true external storage for the contents, meaning it can legitimately be modified externally after the ArrayBuffer is created. If you can find the property/element split bug (I can't, and couldn't find Waldo on IRC), please make the new bug depend on it.
The property/element split bug is bug 586842.
(In reply to Steve Fink [:sfink] from comment #17)
> (In reply to Till Schneidereit [:till] from comment #14)
> > V8 has the function Object::SetIndexedPropertiesToExternalArrayData[1]
> > which, IIUC, can't be implemented using sfink's
> > JS_NewArrayBufferWithContents.
> 
> Yes. I think this is really a separate requirement, though, and should be
> considered after Waldo's property/element split. (That will bring us more in
> alignment with the V8 API in general, btw.)

Thanks for the explanation. I'm aware of the difference in requirements but thought that it might be possible to meet all of them with one API change.

I realize that you're right about the current way indexed properties are stored making that hard, so I've filed bug 726423 for the V8 compatibility. (I don't yet have bug edit rights, so I can't make it depend on bug 586842.)
(In reply to Till Schneidereit [:till] from comment #19)
> Thanks for the explanation. I'm aware of the difference in requirements but
> thought that it might be possible to meet all of them with one API change.

As did I, until I spent some time with the code. Sorry; I didn't mean to imply that you were inappropriately merging them or something.

And after the property/element split, they will hopefully share the same code path.

> I realize that you're right about the current way indexed properties are
> stored making that hard, so I've filed bug 726423 for the V8 compatibility.
> (I don't yet have bug edit rights, so I can't make it depend on bug 586842.)

Done.
Attached patch Add JSAPI for transferring ArrayBuffer contents (obsolete) (deleted) — — Splinter Review 
This introduces an unfortunate speed hit to retrieving the length of TypedArrays.
Attachment #596721 - Flags: review?(dmandelin)
Keywords: dev-doc-needed
We don't want to take any speed hits to basic stuff like retrieving the length unless absolutely necessary. It seems like with the current spec the best way to go is probably to make ArrayBuffers keep a list of their views, and then notify the views if they get truncated. That could still end up slowing down typed array code a bit, because it would need to check for truncation, but we can probably come up with some way of doing that through invalidation.

Or, maybe this is an issue for the spec writers to think about. Not sure.
From an API perspective, I prefer for the length to remain fixed, and gets and sets to indexed properties on a transferred ArrayBuffer to throw. This has better usability and is saner from a types perspective, and IIUC it should improve the performance problem sfink describes here.

I will engage Ken Russell on this again. I started the conversation in December but got distracted by the holidays.

Dave
The current semantics of truncating the lengths of the ArrayBuffer and all of its views were arrived at after pretty careful analysis of the possible options. I agree that it's ugly for the ArrayBuffer to have to refer to all of its views, though.

If anyone has suggestions about improving the semantics to be friendlier to implementations, please do post them to public-webapps@w3.org. Check the archives from April to June 2011 for extensive discussions on Transferable support.
Let me briefly mention that changing the semantics to leave the length as is but throw upon access may impose a run-time performance issue. Each indexed property access would need to do first a length check, and then a neutering check. Perhaps there is a way to merge the two, and optimizing JS VMs may certainly be able to hoist these checks out of loops. In comparison, the semantics of zeroing the length imposes a performance overhead on postMessage and upon typed array view construction. To date, the typed array spec has been focused on allowing getters and setters to be as fast as possible.

Let's have this discussion to public mailing lists though.
I found a bunch of discussion following from http://lists.w3.org/Archives/Public/public-webapps/2011AprJun/0304.html but I couldn't find specific discussion about the neutered length behavior. I'm also not familiar with the standards process, so I'm just going to document my concerns here for now. I could post to a new thread, but I don't want to re-raise issues that have already been discussed just because I can't find them.

Zeroing the length of all views of an ArrayBuffer has either a runtime cost (because the TypedArray or other ArrayBufferView has to check the ArrayBuffer for neutering instead of caching its length) or a complexity cost (back-pointers from an ArrayBuffer to all of its views.) For the indexed element getters and setters, they already have to look through to the ArrayBuffer, so the check is easier/faster; you need to check against the length already. (My naive implementation returns undefined for any lookup on a neutered ArrayBuffer, without adding any code at all to handle it specially, but it looks like it would be straightforward to make them throw instead.)

To be more concrete, here's my pseudocode for TypedArray.length:

  return this.length

And for TypedArray.get:

  buffer = this.arrayBuffer
  if i < buffer.length:
    return buffer[i]
  else:
    if buffer.neutered:
      throw "neutered"
    else:
      return undefined

And for ArrayBuffer.transfer:

  this.neutered = true
  this.length = 0

It's an open question what JS-level ArrayBuffer.length should return -- the original length, like TypedArray, or zero, reflecting the underlying object. If it needs to return the original length, then ArrayBuffer.transfer would instead be:

  this.neutered = true
  this.originalLength = this.length
  this.length = 0

Either way, getters only have to do extra work to handle neutering in the case where the bounds check has already failed.

There are also the semantics of the TypedArray.length field. It's read-only. Is it non-configurable? Are users supposed to expect that such fields will not change? I just don't know what it means for an object like a TypedArray to have a supposedly constant (b/c it's fixed-length) field that can still change value at any time. It's the same issue with ArrayBuffer, only at least there you're doing a mutating operation on it.
Attachment #596721 - Flags: review?(dmandelin)
Do you think it is possible to add a function to set ArrayBuffer to "neutered"( without stealing it)?
It's something which could be nice for an async ParallelArray(rivertrail++) or WebCL.
Sure. But then would you also need to be able to undo the neutering too?

What's the exact use case?

By the way, status on this bug is that I'm effectively treating it as blocked on bug 575688 because there are so many changes to work through there in how typed arrays and array buffers should work in general. So I expect this patch to be substantially rewritten when the final version of 575688 finally lands (and that is itself blocked on evilpie's cleanup in bug 711843, and all of these are dependent on reviews from a very overloaded Waldo.)
Depends on: 575688
(In reply to Steve Fink [:sfink] from comment #28)
> Sure. But then would you also need to be able to undo the neutering too?
It would be great to have that, but it's not in the spec :(


> What's the exact use case?
When I copy the data from ArrayBuffer to OpenCL, I don't know when the read will be performed... So, in order to prevent concurrent accesses, I create a temp copy, which will be used to do the copy in the device memory (for example GPU)

this solution has 2 problems:
- in WebCL,async Read and write are slow with big data (malloc+memcpy)
- memory overhead with the temp copy


===
var myCallback = function(e) {
 fin = new Date()
t1 = fin-debut;
t2 = inter-debut;
};
var buf = new ArrayBuffer(1024*1024*4*12);
var debut = new Date();
***AskAsyncReadBufferWithCB(buf,myCallback );***
var inter = new Date();
===

With copy//clone
t1 = 37 ms (  +- 5 ms)
t2 = 20 ms (  +- 1 ms)


When ArrayBuffer implements transferable*
t1 = 21 ms (  +- 3 ms) 
t2 = 1 ms (  +- 1 ms)
Furthermore the user won't see an unexpected behavior and no extra
memory is needed for the copy.

* it's not exactly transferable :for my test, I add a "neutered" flag which block all change of the ArrayBuffer,from javascript , until all tasks are done.

> By the way, status on this bug is that I'm effectively treating it as
> blocked on bug 575688 because there are so many changes to work through
> there in how typed arrays and array buffers should work in general. So I
> expect this patch to be substantially rewritten when the final version of
> 575688 finally lands (and that is itself blocked on evilpie's cleanup in bug
> 711843, and all of these are dependent on reviews from a very overloaded
> Waldo.)
I don't really need it now. In fact, my message had 2 purposes : having your opinion and showing that the need is not only for webworkers.
Jeff (jgilbert) convinced me that we don't need to be able to undo the neutering :  I can send a new JSObject with the old ArrayBuffer's data in a callback.
Blocks: 783190
Attached patch Add JSAPI for tranferring ArrayBuffer contents (obsolete) (deleted) — — Splinter Review 
Finally got around to updating the patch. It contains a somewhat unfortunate fallout of the semantics in the spec -- specifically, any ArrayBufferView whose ArrayBuffer has been neutered must return 0 for length, byteLength, and byteOffset, and accesses through those views should fail. In order to minimize the performance hit from the possibility of transferrable, that means that all views of an neutered ArrayBuffer need to be updated. Which then implies that the ArrayBuffer has a back pointer to all of its views, and those back pointers must be weak, so the pointers must be cleaned up when a view is GCed.

The end results are (1) ArrayBuffer objects need a pointer to a view list somewhere, and (2) views need a finalizer to do cleanup. (1) is particularly messy with the current object layout, since the only decent place to put it is in the ObjectElements header. (The private field is already used for a delegate object.) It can be done fairly cleanly in Waldo's new object layout.

(2) means that ArrayBufferViews can no longer be swept in the background, which could be potentially significant if there are many small typed arrays floating around. In the future, it might make sense to add an additional level of weakness by tracing through the view list during regular GCs and finalizing them during compacting GCs (or some other occasional GC.)
Attachment #596721 - Attachment is obsolete: true
Attachment #652522 - Flags: review?(luke)
Comment on attachment 652522 [details] [diff] [review]
Add JSAPI for tranferring ArrayBuffer contents

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

One non-trivial comment below:

::: js/src/jsapi.h
@@ +4824,5 @@
> + * JS_NewArrayBufferWithContents. |nbytes| is the number of payload bytes
> + * required. The pointer to pass to JS_NewArrayBufferWithContents is returned
> + * in |contents|. The pointer to the |nbytes| of usable memory is returned in
> + * |data|. (*|contents| will contain a header before |data|.) The only legal
> + * operations on *|contents| is to free it or pass it to

To accentuate this point, could you make 'contents' (in all three signatures) be a void **?

::: js/src/jsobj.h
@@ +560,5 @@
>  
>      inline bool ensureElements(JSContext *cx, unsigned cap);
>      bool growElements(JSContext *cx, unsigned cap);
>      void shrinkElements(JSContext *cx, unsigned cap);
> +    void setDynamicElements(js::ObjectElements *header) { this->elements = header->elements(); }

Is there something you can assert about this->elements before (like its being NULL)?  If this grows to more than 1 line, could you put it next to the other *elements functions in setDynamicElements?

::: js/src/jstypedarray.cpp
@@ +1029,5 @@
>      static void
> +    obj_finalize(FreeOp *fop, JSObject *obj)
> +    {
> +        JS_ASSERT(obj->hasClass(fastClass()));
> +        buffer(obj)->asArrayBuffer().removeView(fop, obj);

Is there a hazard here if, during finalization, the buffer is also being dead and already finalized by the time obj_finalize is called and thus calling removeView will act poorly?

On first glance, this is hard to get around without a special "sweep typed arrays" phase.  What if, instead, we gave an extra slot to array buffers which is either a direct pointer to its one view or a pointer to a malloc'd array of views.  I would expect the majority of array buffers are never aliased with a second view and thus we could avoid the malloc.
Attachment #652522 - Flags: review?(luke)
(In reply to Luke Wagner [:luke] from comment #32)
> Comment on attachment 652522 [details] [diff] [review]
> ::: js/src/jstypedarray.cpp
> @@ +1029,5 @@
> >      static void
> > +    obj_finalize(FreeOp *fop, JSObject *obj)
> > +    {
> > +        JS_ASSERT(obj->hasClass(fastClass()));
> > +        buffer(obj)->asArrayBuffer().removeView(fop, obj);
> 
> Is there a hazard here if, during finalization, the buffer is also being
> dead and already finalized by the time obj_finalize is called and thus
> calling removeView will act poorly?

Ah, good catch. But couldn't I use IsAboutToBeFinalized() to check that?

> On first glance, this is hard to get around without a special "sweep typed
> arrays" phase.  What if, instead, we gave an extra slot to array buffers
> which is either a direct pointer to its one view or a pointer to a malloc'd
> array of views.  I would expect the majority of array buffers are never
> aliased with a second view and thus we could avoid the malloc.

ArrayBufferObjects' slot storage is used for the data, so I don't think I can give them an extra slot. Unless you just mean carve a "slot" out of the header, as this patch already does, but redefine it to mean either a direct view pointer or a malloced array of views? That would save a slot in every view in the common case.
(In reply to Steve Fink [:sfink] from comment #33)
> Ah, good catch. But couldn't I use IsAboutToBeFinalized() to check that?

I dunno, can you?

> Unless you just mean carve a "slot" out of the
> header, as this patch already does, but redefine it to mean either a direct
> view pointer or a malloced array of views? That would save a slot in every
> view in the common case.

That's right.
Attached patch Add JSAPI for transferring ArrayBuffer contents (obsolete) (deleted) — — Splinter Review 
(In reply to Luke Wagner [:luke] from comment #34)
> (In reply to Steve Fink [:sfink] from comment #33)
> > Ah, good catch. But couldn't I use IsAboutToBeFinalized() to check that?
> 
> I dunno, can you?

Seems like it.

> > Unless you just mean carve a "slot" out of the
> > header, as this patch already does, but redefine it to mean either a direct
> > view pointer or a malloced array of views? That would save a slot in every
> > view in the common case.
> 
> That's right.

Ugh. Then you deserve to have to review this new patch.

This patch also changes the object creation paths. It turns out that simply adding a finalizer to a JSClass isn't enough to get it finalized, at least not when using the bizarre and twisted object creation path that typed arrays go through. They were creating new objects using the prototype class, then mutating them into the instance class. Which never made sense to me, but it also means that the finalization kind is based on the prototype class, and the prototype is background finalizable.

Crud, that means I ought to be adding an assert somewhere. I filed bug 783708 for that, since I'm unsure of how exactly that should work.

bhackett seems to be saying these object creation changes are valid over in bug 761506. The TI stuff is still way too clunky, but I'll leave it alone for now.
Attachment #652966 - Flags: review?(luke)
Attachment #652522 - Attachment is obsolete: true
(In reply to Steve Fink [:sfink] from comment #35)
> Ugh. Then you deserve to have to review this new patch.

Yeah, after looking at it I'm starting to think the original approach was better (fixed with IsAboutToBeFinalized() as you said in comment 33).  If we really want to shrink typed array views, we should do lazy creation of buffers as we discussed earlier which should save a good bit more memory than this.
Ok, back to the original, with IsAboutToBeFinalized and the better tests.
Attachment #654812 - Flags: review?(luke)
Attachment #652966 - Attachment is obsolete: true
Attachment #652966 - Flags: review?(luke)
Blocks: 785167
Comment on attachment 654812 [details] [diff] [review]
Add JSAPI for transferring ArrayBuffer contents

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

::: js/src/jsapi.h
@@ +4816,5 @@
> + * of |contents| and must free it or transfer ownership via
> + * JS_NewArrayBufferWithContents when done using it.
> + */
> +extern JS_PUBLIC_API(JSBool)
> +JS_StealArrayBufferContents(JSContext *cx, JSObject *obj, uint8_t **contents);

repeating nit from comment 32

::: js/src/jstypedarray.cpp
@@ +245,5 @@
>      return true;
>  }
>  
> +static uint32_t
> +nextViewSlot(JSObject *obj)

I think style is to capitalize the first letter of static functions.

@@ +251,5 @@
> +    return obj->isTypedArray() ? TypedArray::FIELD_NEXT_VIEW : DataViewObject::NEXT_VIEW_SLOT;
> +}
> +
> +static JSObject **
> +getViewList(JSObject *obj)

ditto

@@ +262,5 @@
> +    // the slots are already being used for the element storage and the private
> +    // field is used for a delegate object. The ObjectElements header has space
> +    // for it, but I don't want to mess around with adding unions to it with
> +    // USE_NEW_OBJECT_REPRESENTATION pending, since it will solve this much
> +    // more cleanly.

hah, I like your style

@@ +276,5 @@
> +void
> +ArrayBufferObject::addView(JSContext *cx, RawObject view)
> +{
> +    JSObject **views = getViewList(this);
> +    view->setFixedSlot(nextViewSlot(view), *views ? ObjectValue(**views) : NullValue());

You can use ObjectOrNullValue(*views) instead.

@@ +281,5 @@
> +    *views = view;
> +}
> +
> +void
> +ArrayBufferObject::removeView(FreeOp *fop, RawObject view)

This is a special function that can only be called during finalization.  How about naming it removeFinalizedView?  Second, perhaps a comment about why we it is ok to traverse the linked list of views during finalization (already-finalized views have already been removed, to-be-finalized views are still valid).

@@ +295,5 @@
> +    while (linkObj) {
> +        uint32_t linkObjSlot = nextViewSlot(linkObj);
> +        JSObject *next = linkObj->getFixedSlot(linkObjSlot).toObjectOrNull();
> +        if (next == view)
> +            linkObj->setFixedSlot(linkObjSlot, next->getFixedSlot(nextViewSlot(next)));

Can you 'break' here?  Then there wouldn't be an else branch and what you'd really have was:

  for (JSObject *linkObj = *views; linkObj; linkObj = next)

but, really, will the loop condition ever be false?  I'd rather leave it out so that we get a clean NULL-deref crash if we try to remove a view that isn't in the ArrayBuffer's list.

Also, this code really wants an ArrayBufferView JSObject-subclass to avoid all the manual get/setFixedSlot.  I guess that ship has already sailed...  how about NextView/SetNextView static helper functions; I think there are several uses here and below.

@@ +392,5 @@
> +        uint32_t length = buffer.byteLength();
> +        js::ObjectElements *newheader =
> +            AllocateArrayBufferContents(cx, length, buffer.dataPointer());
> +        if (!newheader)
> +            js_ReportOutOfMemory(cx);

... and return false

@@ +1404,5 @@
>          obj->setSlot(FIELD_BYTEOFFSET, Int32Value(byteOffset));
>          obj->setSlot(FIELD_BYTELENGTH, Int32Value(len * sizeof(NativeType)));
> +        obj->setSlot(FIELD_NEXT_VIEW, NullValue());
> +
> +        if (!obj->preventExtensions(cx))

Good, I did not care for that previous tomfoolery.

@@ +3510,5 @@
> +JS_PUBLIC_API(JSObject *)
> +JS_NewArrayBufferWithContents(JSContext *cx, uint8_t *contents)
> +{
> +    JSObject *obj = ArrayBufferObject::create(cx, 0);
> +    obj->setDynamicElements(reinterpret_cast<js::ObjectElements *>(contents));

null check

@@ +3519,5 @@
> +JS_AllocateArrayBufferContents(JSContext *cx, uint32_t nbytes, uint8_t **contents, uint8_t **data)
> +{
> +    js::ObjectElements *header = AllocateArrayBufferContents(cx, nbytes, NULL);
> +    if (!header)
> +        return JS_FALSE;

can you use 'false' and 'true' here and below?
Attachment #654812 - Flags: review?(luke) → review+
(In reply to Luke Wagner [:luke] from comment #38)
> Comment on attachment 654812 [details] [diff] [review]
> Add JSAPI for transferring ArrayBuffer contents
> 
> Review of attachment 654812 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsapi.h
> @@ +4816,5 @@
> > + * of |contents| and must free it or transfer ownership via
> > + * JS_NewArrayBufferWithContents when done using it.
> > + */
> > +extern JS_PUBLIC_API(JSBool)
> > +JS_StealArrayBufferContents(JSContext *cx, JSObject *obj, uint8_t **contents);
> 
> repeating nit from comment 32

Oh! Sorry. I was thinking too much about partially reverting to my previous patch, and totally forgot that there were some comments outstanding.

(In reply to Luke Wagner [:luke] from comment #32)
> Comment on attachment 652522 [details] [diff] [review]
> Add JSAPI for tranferring ArrayBuffer contents
> 
> Review of attachment 652522 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> One non-trivial comment below:
> 
> ::: js/src/jsapi.h
> @@ +4824,5 @@
> > + * JS_NewArrayBufferWithContents. |nbytes| is the number of payload bytes
> > + * required. The pointer to pass to JS_NewArrayBufferWithContents is returned
> > + * in |contents|. The pointer to the |nbytes| of usable memory is returned in
> > + * |data|. (*|contents| will contain a header before |data|.) The only legal
> > + * operations on *|contents| is to free it or pass it to
> 
> To accentuate this point, could you make 'contents' (in all three
> signatures) be a void **?

Done.

> ::: js/src/jsobj.h
> @@ +560,5 @@
> >  
> >      inline bool ensureElements(JSContext *cx, unsigned cap);
> >      bool growElements(JSContext *cx, unsigned cap);
> >      void shrinkElements(JSContext *cx, unsigned cap);
> > +    void setDynamicElements(js::ObjectElements *header) { this->elements = header->elements(); }
> 
> Is there something you can assert about this->elements before (like its
> being NULL)?  If this grows to more than 1 line, could you put it next to
> the other *elements functions in setDynamicElements?

Ok. I asserted !hasDynamicElements before and the hasDynamicElements after. (The latter will slap the wrist of someone trying to use setDynamicElements to switch to inline elements.)

(In reply to Luke Wagner [:luke] from comment #38)
> @@ +245,5 @@
> >      return true;
> >  }
> >  
> > +static uint32_t
> > +nextViewSlot(JSObject *obj)
> 
> I think style is to capitalize the first letter of static functions.

Oh, right. Not sure how I missed that. Done.

> @@ +276,5 @@
> > +void
> > +ArrayBufferObject::addView(JSContext *cx, RawObject view)
> > +{
> > +    JSObject **views = getViewList(this);
> > +    view->setFixedSlot(nextViewSlot(view), *views ? ObjectValue(**views) : NullValue());
> 
> You can use ObjectOrNullValue(*views) instead.

Oh, nice.

> @@ +281,5 @@
> > +    *views = view;
> > +}
> > +
> > +void
> > +ArrayBufferObject::removeView(FreeOp *fop, RawObject view)
> 
> This is a special function that can only be called during finalization.  How
> about naming it removeFinalizedView?  Second, perhaps a comment about why we
> it is ok to traverse the linked list of views during finalization
> (already-finalized views have already been removed, to-be-finalized views
> are still valid).

Done.

> 
> @@ +295,5 @@
> > +    while (linkObj) {
> > +        uint32_t linkObjSlot = nextViewSlot(linkObj);
> > +        JSObject *next = linkObj->getFixedSlot(linkObjSlot).toObjectOrNull();
> > +        if (next == view)
> > +            linkObj->setFixedSlot(linkObjSlot, next->getFixedSlot(nextViewSlot(next)));
> 
> Can you 'break' here?  Then there wouldn't be an else branch and what you'd
> really have was:
> 
>   for (JSObject *linkObj = *views; linkObj; linkObj = next)
> 
> but, really, will the loop condition ever be false?  I'd rather leave it out
> so that we get a clean NULL-deref crash if we try to remove a view that
> isn't in the ArrayBuffer's list.

Ok. I waffled on this earlier, but didn't consider letting it walk off the list. That's a good idea.

It still ain't pretty.

> Also, this code really wants an ArrayBufferView JSObject-subclass to avoid
> all the manual get/setFixedSlot.  I guess that ship has already sailed...

I still intend to do that someday. I took a stab at it for this patch, but it got messy and was clearly going to take a while, so I wimped out.
 
> how about NextView/SetNextView static helper functions; I think there are
> several uses here and below.

Ok. You're right; it cleared things up a little.

> 
> @@ +392,5 @@
> > +        uint32_t length = buffer.byteLength();
> > +        js::ObjectElements *newheader =
> > +            AllocateArrayBufferContents(cx, length, buffer.dataPointer());
> > +        if (!newheader)
> > +            js_ReportOutOfMemory(cx);
> 
> ... and return false

You're so negative.

> @@ +3510,5 @@
> > +JS_PUBLIC_API(JSObject *)
> > +JS_NewArrayBufferWithContents(JSContext *cx, uint8_t *contents)
> > +{
> > +    JSObject *obj = ArrayBufferObject::create(cx, 0);
> > +    obj->setDynamicElements(reinterpret_cast<js::ObjectElements *>(contents));
> 
> null check

Done.

> @@ +3519,5 @@
> > +JS_AllocateArrayBufferContents(JSContext *cx, uint32_t nbytes, uint8_t **contents, uint8_t **data)
> > +{
> > +    js::ObjectElements *header = AllocateArrayBufferContents(cx, nbytes, NULL);
> > +    if (!header)
> > +        return JS_FALSE;
> 
> can you use 'false' and 'true' here and below?

I guess I've had this patch kicking around for a long time. (Not that it was correct when I wrote this part, but I hadn't changed habits yet.)
https://hg.mozilla.org/mozilla-central/rev/102c2795bacc
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 787775
Depends on: CVE-2014-1514
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: