1168471 - Add support to WebIDL generator to handle SharedArrayBuffer and SharedArrayViews.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Jukka Jylänki]

There's need to expand existing WebIDL-based web interfaces to take in SAB types. This is a prerequisite for bug 1147441.

Comment 1

[Jukka Jylänki]

Attachment: webidl-sab.diff

Patch to implement SAB support in WebIDL.

Comment 2

[Jukka Jylänki]

Not sure who would be the most correct person to review this - bzbarsky helped me implement it, though his handle mentions he's not taking reviews right now. Luke/Lars, feel free to reassign if you know who's better suited? Also, please bear with me, this is my first jump from behind the Emscripten land to Firefox code tree :)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8610681 [details] [diff] [review]
webidl-sab.diff

You need to add these new types to dom/bindings/parser/tests/test_distinguishability.py in the "test our whole distinguishability table" section, and add tests for their behavior there.  And I guess add some of the existing types too, since they aren't there now.

I say this because the IDLBuiltinType.isDistinguishableFrom implementation looks wrong: if self is a typed array and other is a shared array buffer view it will claim distinguishable, but I would think it should be non-distinguishable.

On the other hand, maybe Int8Array and SharedArrayBufferView should in fact be distinguishable... but earlier in that function if self is a shared array buffer view and other is a typed array we claim non-distinguishable.  This asymmetry is clearly a bug, and the test would have caught it.

Also, does js::GetInt8ArrayLengthAndData really work correctly on a SharedInt8Array while JS_GetInt8ArrayData doesn't?  That's a bit surprising at best, and I expect it's just not true.

Please do make sure you get review from someone familiar with the codegen bits for this.

Comment 4

[Jukka Jylänki]

(In reply to Not doing reviews right now from comment #3)
> Comment on attachment 8610681 [details] [diff] [review]
> webidl-sab.diff
> 
> You need to add these new types to
> dom/bindings/parser/tests/test_distinguishability.py in the "test our whole
> distinguishability table" section, and add tests for their behavior there. 
> And I guess add some of the existing types too, since they aren't there now.
> 
> I say this because the IDLBuiltinType.isDistinguishableFrom implementation
> looks wrong: if self is a typed array and other is a shared array buffer
> view it will claim distinguishable, but I would think it should be
> non-distinguishable.

Thanks for the comment bzbarsky!

I updated the test now, and added ArrayBuffer, ArrayBufferView, SharedArrayBuffer and SharedArrayBufferViews there. However I'm struggling a bit to understand the concept of distinguishability. The WebIDL spec at http://heycam.github.io/webidl/#idl-overloading talks a lot about distinguishability and when two types are distinguishable, but it does not actually explain what it means, and how two types behave if they are distinguishable from each other.

My interpretation of distinguishability is that if two types are distinguishable, then it is allowed/possible to create multiple overloads of a function with the same name, and the implementation is able to pick which one to call. When I look at the WebIDL spec and compare it with the current implementation, I see a discrepancy: the table in the spec says that an object of category "buffer source types" is not distinguishable with itself. However in the current webidl.py implementation, ArrayBuffer and ArrayBufferView are distinguishable between each other, even though they both are "buffer source types". Perhaps I interpret the spec oddly.

With respect to the SharedArrayBuffer and SharedArrayView: Lars has specced that whenever a WebIDL function takes in an ArrayBuffer, then passing a SharedArrayBuffer (or vice versa) is not automatically allowed. Likewise, when a function signature takes in a ArrayView, passing a SharedArrayView is not allowed (and vice versa). The intent is to prohibit data raciness from automatically "expanding" through the APIs, but all APIs should explicitly notice where potentially shared data is being passed. To me that sounds like each of the types ArrayBuffer, SharedArrayBuffer, ArrayView and SharedArrayView should be distinguishable from each other (except itself). I updated the patch to add a test in dom/bindings/parser/tests/test_distinguishability.py according to this understanding. The current implementation agrees with the test.

> On the other hand, maybe Int8Array and SharedArrayBufferView should in fact
> be distinguishable... but earlier in that function if self is a shared array
> buffer view and other is a typed array we claim non-distinguishable.  This
> asymmetry is clearly a bug, and the test would have caught it.

Distinguishability should be a symmetric property, and according to the test I added, I understand that the implementation should maintain that it is so, and the test should check. Can you double-check if I did a flaw in the test?

> Also, does js::GetInt8ArrayLengthAndData really work correctly on a
> SharedInt8Array while JS_GetInt8ArrayData doesn't?  That's a bit surprising
> at best, and I expect it's just not true.
> 
> Please do make sure you get review from someone familiar with the codegen
> bits for this.

If you are referring to these lines:

+typedef TypedArray<int8_t, js::UnwrapSharedInt8Array, JS_GetSharedInt8ArrayData,
+                   js::GetInt8ArrayLengthAndData, JS_NewSharedInt8Array>
+        SharedInt8Array;

then the choice here was to reuse the js::GetInt8ArrayLengthAndData as-is instead of copy-pasting them to duplicate js::GetSharedInt8ArrayLengthAndData, since the implementation was identical for both.

Updated patch below.

Comment 5

[Jukka Jylänki]

Attachment: webidl-sab-with-test.diff

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8610752 [details] [diff] [review]
webidl-sab-with-test.diff

I'm not really familiar enough with the WebIDL code to review this, but at a high level what you're doing makes sense.

At the same time, there's been some recent discussion of whether we really need a separate type hierarchy for Shared*Views or whether the same views can be used for both ABs and SABs.  If that changes, then this can change too, since it's all experimental, but perhaps Lars has more recent info.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> My interpretation of distinguishability

Is correct.

> the table in the spec says that an object of category "buffer source types" is not
> distinguishable with itself

That's clearly a bug in the Web IDL spec, since the BufferSource typedef, for example, requires ArrayBufferView to be distinguishable from ArrayBuffer.  Filed https://github.com/heycam/webidl/issues/50

> To me that sounds like each of the types ArrayBuffer, SharedArrayBuffer, ArrayView and
> SharedArrayView should be distinguishable from each other (except itself)

Yes, agreed.

> If you are referring to these lines:

Yes, I am.

> since the implementation was identical for both.

And the MOZ_ASSERT there about the Class of the object passes with shared arrays?

Comment 8

[Boris Zbarsky [:bzbarsky]]

The distinguishability test should also include Uint8Array and SharedUint8Array.  And maybe, for good measure, Int16Array and SharedInt16Array...  If you do that, you should get test failures given the code in webidl.py.

Comment 9

[Lars T Hansen [:lth]]

Not clear to me how the code in this function this makes sense:

+JS_FRIEND_API(void)
+js::GetSharedArrayBufferViewLengthAndData(JSObject* obj, uint32_t* length, uint8_t** data)

The function starts by asserting that the object is a SharedArrayViewObject but then goes into some song and dance with DataView.  It should not be possible to apply DataView to shared memory (in the current design).

Re Luke's remark in comment #6, that discussion is stalled but I will restart it.  At the moment, the shared/unshared type hierarchies are completely disjoint.

Comment 10

[Lars T Hansen [:lth]]

Comment on attachment 8610752 [details] [diff] [review]
webidl-sab-with-test.diff

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

Probably OK with me though like Luke I don't really know WebIDL enough to be qualified to have an opinion.

There are some nits that need to be addressed.

::: js/src/vm/ArrayBufferObject.cpp
@@ +1289,5 @@
> +JS_FRIEND_API(JSObject*)
> +js::UnwrapSharedArrayBufferView(JSObject* obj)
> +{
> +    if (JSObject* unwrapped = CheckedUnwrap(obj))
> +        return unwrapped->is<SharedTypedArrayObject>() ? unwrapped : nullptr; /// XXX SharedArrayBufferViewObject?

Comment?

::: js/src/vm/SharedArrayObject.cpp
@@ +388,5 @@
> +    MOZ_ASSERT(obj->is<SharedTypedArrayObject>());
> +
> +    *length = obj->is<DataViewObject>()
> +              ? obj->as<DataViewObject>().byteLength()
> +              : obj->as<SharedTypedArrayObject>().byteLength();

I don't understand what the DataViewObject is doing in here.  There are no DataViews on shared memory.

::: js/src/vm/TypedArrayObject.cpp
@@ +2206,5 @@
>  
>      if (obj->is<TypedArrayObject>())
>          return obj->as<TypedArrayObject>().type();
> +    else if (obj->is<SharedTypedArrayObject>())
> +        return obj->as<SharedTypedArrayObject>().type();

I'm not real fond of mixing the shared types in with the unshared types.  For my money, there would be a JS_GetSharedArrayBufferViewType() method.

Comment 11

[Jukka Jylänki]

Thanks for the comments!

I adapted the patch to address the above reviews:

 1. Added tests for distinguishability of (Shared)Uint8/16Arrays, from comment 8.
 2. Fixed the distinguishability logic after adding the test, like mentioned in comment 8.
 3. Removed references to DataView in the patch, comments 9 and 10.
 4. Removed the temp // XXX comment in the patch related to naming, comment 10.
 5. Added a new function JS_GetSharedArrayBufferViewType() to ask the type of STA separately, comment 10.

I did a ac_add_options --enable-debug build after these changes and ran the Unity WebGL benchmark with SAB/pthreads enabled to verify that the code works. New updated patch below. How does that look?

Comment 12

[Jukka Jylänki]

Attachment: webidl-sab-sta.diff

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8615704 [details] [diff] [review]
webidl-sab-sta.diff

I still don't see how js::GetInt8ArrayLengthAndData can possibly work correctly on a SharedInt8Array.  The first thing it does is assert that GetObjectClass(obj) == detail::Int8ArrayClassPtr, as far as I can tell.  Similar for the other Get*ArrayLengthAndData functions.  I've mentioned this before in this bug... am I just missing something?

>     def isTypedArray(self):
>-        return self._typeTag >= IDLBuiltinType.Types.Int8Array and \
>-               self._typeTag <= IDLBuiltinType.Types.Float64Array
>+        return (self._typeTag >= IDLBuiltinType.Types.Int8Array and \
>+               self._typeTag <= IDLBuiltinType.Types.Float64Array)

Please undo this change.  And after that make the isSharedTypeArray case look like this one in terms of indentation and parens.

>dom/bindings/parser/tests/test_distinguishability.py

>+    nonUserObjects = nonObjects + interfaces + dates + sequences + bufferSourceTypes + sharedBufferSourceTypes

So what I think you should do, actually, is include bufferSourceTypes + sharedBufferSourceTypes in the "interfaces" list and explicitly in the notRelatedInterfaces list, and _not_ include it explicitly in nonUserObjects.

Then you won't run into the issue that right now the list you pass to setDistinguishable for "Interface" contains both notRelatedInterfaces and bufferSourceTypes but your definition of notRelatedInterfaces includes otherObjects, which already includes the bufferSourceTypes...  So there's extra work being done by the test for the duplicate entries in that list.

Anyway, if you just include the buffer sources (shared and not) in "interfaces" and "notRelatedInterfaces" and leave the definitions of nonUserObjects and otherObjects alone, then you shouldn't need any changes to any existing setDistinguishable calls at all, just the addition of the new ones.

r=me on the dom/bindings/parser changes with the above nits, but please do fix the bit in dom/bindings/TypedArray.h by using functions that actually work on shared arrays/arraybuffers.

Comment 14

[Jukka Jylänki]

Thanks bzbarsky, addressed the changes from comment 13:
   - Remove redundant parentheses in isTypedArray and aligned iSharedTypedArray similarly
   - Changed test_distinguishability.py to include (shared)bufferSourceTypes in interfaces and notRelatedInterfaces, and removed it from nonUserObjects
   - Changed the Shared*Array to use the form js::GetShared*ArrayData instead of js:Get*ArrayData. Thanks for persisting on this, I first misread the code.

New patch below. I assume bzbarsky should be ok, so r? from Lars only to give a second look to the changes.

Comment 15

[Jukka Jylänki]

Attachment: webidl-sab-sta.diff

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8616448 [details] [diff] [review]
webidl-sab-sta.diff

>-    otherObjects = allBut(argTypes, nonUserObjects + ["object"])
>+    otherObjects = allBut(argTypes, nonObjects + interfaces + dates + sequences + ["object"])

This change is no longer needed, given:

     nonUserObjects = nonObjects + interfaces + dates + sequences

Looks good otherwise.

Comment 17

[Jukka Jylänki]

Thanks, updated.

Comment 18

[Jukka Jylänki]

Attachment: webidl-sab-sta.diff

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Is there a Try link handy for this patch? :)

Comment 20

[Jukka Jylänki]

I do not have try access, and obtaining one devolved down to some infra debugging on an issue that new ssh accesses aren't being properly granted (bug 1174146). That has not been resolved yet, but Lars helped here and spun up a try build, which is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4987fa770315

There was some warning about the order of #include directives in one of the files. Updated the patch to fix it (just swapped the order of two #include lines, and tested locally that it still builds). Lars, sorry to bug you again, but could you spin up another try build of the new patch for Ryan?

Comment 21

[Jukka Jylänki]

Attachment: webidl_sab_sta.diff

Comment 22

[Lars T Hansen [:lth]]

remote: View your changes here:
remote:   https://hg.mozilla.org/try/rev/84407b8f88f8
remote:   https://hg.mozilla.org/try/rev/ba897cc5c0b8
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba897cc5c0b8

Comment 23

[Jukka Jylänki]

Thanks Lars!

Looking at the results, everything is green except for one test that timed out, for https://bugzilla.mozilla.org/show_bug.cgi?id=706743 which seems to be unrelated. I think this should be all good to go.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ecfd97724ed

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6ecfd97724ed