971845 - Add new JS_GetStableArrayBufferData that uninlines and always takes a cx

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: rm-maybecx

JS_GetArrayBufferData doesn't take a cx but does always un-inline.  This is the only case where a null maybecx can flow into all the functions in vm/TypedArrayObject.cpp and it'd be really nice to remove this possibility.  This patch makes JS_GetArrayBufferData never uninline and adds a JS_GetStableArrayBufferData for the cases that actually do need a stable array that is valid across GCs.  This is symmetric with JS_GetInt32ArrayData (and the 8 other variants) that do not uninline and do not take a cx.  I manually audited all the calls to JS_GetArrayBufferData and changed to Stable all the ones that didn't obviously drop the buffer pointer before a possible GC.

A nice benefit is that this patch optimizes all the places that can still use JS_GetStableArrayBufferData.

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 8374973 [details] [diff] [review]
rm-maybecx

... and try reveals someone was calling JS_ReallocateArrayBufferContents passing a null 'cx' arg.  Easy enough to fix; trying again.

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8374973 [details] [diff] [review]
rm-maybecx

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

Oh. Looks like this one has become obsolete. Most of my comments should still hold.

::: dom/workers/FileReaderSync.cpp
@@ +75,5 @@
>    }
>  
>    uint32_t bufferLength = JS_GetArrayBufferByteLength(jsArrayBuffer);
> +  uint8_t* arrayBuffer = JS_GetStableArrayBufferData(aCx, jsArrayBuffer);
> +  if (arrayBuffer) {

You might want to check the parity of your boolean there. :-)

::: js/src/jsfriendapi.h
@@ +1396,5 @@
>   * Return a pointer to the start of the data referenced by a typed array. The
>   * data is still owned by the typed array, and should not be modified on
> + * another thread. Furthermore, the pointer can become invalid on GC (if the
> + * data is small and fits inside the array's GC header), so callers must take
> + * care not to hold on too long.

I'd be even more explicit. "not to hold on too long" -> "not to hold on across anything that could GC."

@@ +1426,5 @@
>  JS_GetFloat64ArrayData(JSObject *obj);
>  
>  /*
> + * Stable versions of the above functions where the buffer remains valid as long
> + * as the object is rooted.

Technically, this probably ought to be "live", not "rooted", since it can be reachable other ways. But this is probably fine, since it seems to be the common way to refer to it.

::: js/src/vm/TypedArrayObject.cpp
@@ +230,5 @@
>   * it's expected to be a previously-allocated ObjectElements* pointer that we
>   * then realloc.
>   */
>  static ObjectElements *
> +AllocateArrayBufferContents(JSContext *cx, uint32_t nbytes, void *oldptr = nullptr)

Sadly, I don't think we can do this. There are callers of JS_AllocateArrayBufferContents that do not have a cx (and are possibly running on a thread without a runtime at all.) See bug 970253.

But we don't need to tell the jit in that case, so I think it's ok.

@@ +400,5 @@
>          uintptr_t newDataPtr = uintptr_t(view->getPrivate()) - oldDataPointer + newDataPointer;
>          view->setPrivate(reinterpret_cast<uint8_t*>(newDataPtr));
>  
>          // Notify compiled jit code that the base pointer has moved.
> +        MarkObjectStateChange(cx, view);

\o/

@@ +4035,4 @@
>  }
>  
>  JS_FRIEND_API(uint8_t *)
>  JS_GetArrayBufferData(JSObject *obj)

I'm half inclined to rename all these JS_GetUnstable... but never mind.

Comment 3

[Luke Wagner [:luke]]

(In reply to Steve Fink [:sfink] from comment #2)
> Sadly, I don't think we can do this. There are callers of
> JS_AllocateArrayBufferContents that do not have a cx (and are possibly
> running on a thread without a runtime at all.) See bug 970253.

mxr doesn't show any uses nor does try server.  To support the off-any-thread use case, it seems like we'd either add a new overload (that doesn't take 'cx') or, and I think this is more attractive, use the approach they are taking in bug 945152 where the caller allocates the memory however they like and then just calls JSAPI methods to stamp the header in place.

Comment 4

[Luke Wagner [:luke]]

Attachment: rm-cruft

Remove dead code.

Comment 5

[Luke Wagner [:luke]]

Attachment: rm-maybecx

Well, maybecx in AllocateArrayBufferContents isn't so bad for now, so disregard comment 3.

Comment 6

[Luke Wagner [:luke]]

Attachment: rm-maybecx

...and with the previous change I can revert my changes to nsXMLHttpRequest.

Comment 7

[Steve Fink [:sfink] [:s:]]

(In reply to Luke Wagner [:luke] from comment #3)
> (In reply to Steve Fink [:sfink] from comment #2)
> > Sadly, I don't think we can do this. There are callers of
> > JS_AllocateArrayBufferContents that do not have a cx (and are possibly
> > running on a thread without a runtime at all.) See bug 970253.
> 
> mxr doesn't show any uses nor does try server.  To support the
> off-any-thread use case, it seems like we'd either add a new overload (that
> doesn't take 'cx') or, and I think this is more attractive, use the approach
> they are taking in bug 945152 where the caller allocates the memory however
> they like and then just calls JSAPI methods to stamp the header in place.

I don't know of any users other than an upcoming one in bug 970253. I thought roc was using it for audio buffers of some sort, but perhaps that's changed or something.

I guess I'm not opposed to this, though it sounds a little dangerous. What are the restrictions on the caller-allocated memory? Clearly, it needs to have space for the header, and hopefully that size won't change much. What alignment does it need, and will we ever need to change it? Do we remember that it was externally-allocated memory in case we need to treat it different for mprotecting or decommitting or whatever? What threads are allowed to touch it? (Not that that really gets any easier going through our allocation policy.) None of these are huge deals, but at some point I think we'd really like to be able to take in just an external data chunk without a header, and then this will all be different again.

On the other hand, it doesn't seem like there are very many users, so it's not like it's a big deal to go back and fix them up as needed. So we don't need to block progress by being too conservative.

(And now that I've said my thing, I'll go review your patch that sidesteps this whole argument...)

Comment 8

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8375216 [details] [diff] [review]
rm-cruft

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

I'm surprised this stuff never gets used. Cool.

Comment 9

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8375624 [details] [diff] [review]
rm-maybecx

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

::: js/src/vm/TypedArrayObject.cpp
@@ +4118,2 @@
>  {
> +    js::ObjectElements *header = AllocateArrayBufferContents(maybecx, nbytes, *contents);

Looks like your code is slightly old -- I just made this change in bug 970253 (b62d2430bcde).

Comment 10

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47870c0b90ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/91edf542ecf4

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47870c0b90ba
https://hg.mozilla.org/mozilla-central/rev/91edf542ecf4