Add new JS_GetStableArrayBufferData that uninlines and always takes a cx

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla30
All
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Posted patch rm-maybecx (obsolete) — Splinter Review
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.
Attachment #8374973 - Flags: review?(sphink)
Attachment #8374973 - Flags: review?(bzbarsky)
Blocks: 936236
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.
Attachment #8374973 - Attachment is obsolete: true
Attachment #8374973 - Flags: review?(sphink)
Attachment #8374973 - Flags: review?(bzbarsky)
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.
(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.
Posted patch rm-cruftSplinter Review
Remove dead code.
Assignee: nobody → luke
Status: NEW → ASSIGNED
Attachment #8375216 - Flags: review?(sphink)
Posted patch rm-maybecx (obsolete) — Splinter Review
Well, maybecx in AllocateArrayBufferContents isn't so bad for now, so disregard comment 3.
Attachment #8375543 - Flags: review?(sphink)
Posted patch rm-maybecxSplinter Review
...and with the previous change I can revert my changes to nsXMLHttpRequest.
Attachment #8375543 - Attachment is obsolete: true
Attachment #8375543 - Flags: review?(sphink)
Attachment #8375624 - Flags: review?(sphink)
(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 on attachment 8375216 [details] [diff] [review]
rm-cruft

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

I'm surprised this stuff never gets used. Cool.
Attachment #8375216 - Flags: review?(sphink) → review+
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).
Attachment #8375624 - Flags: review?(sphink) → review+
https://hg.mozilla.org/mozilla-central/rev/47870c0b90ba
https://hg.mozilla.org/mozilla-central/rev/91edf542ecf4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.