1147441 - Add SharedArrayBuffer support for WebGL.

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, enhancement)

Description

[Jukka Jylänki]

In WebGL 1 specification, the following parts refer to Typed Arrays:

  typedef (ArrayBuffer or ArrayBufferView) BufferDataSource;

  bufferData(..., BufferDataSource? data, ...);
  bufferSubData(..., BufferDataSource? data);
  compressedTexImage2D(..., ArrayBufferView data);
  compressedTexSubImage2D(..., ArrayBufferView data);
  readPixels(..., ArrayBufferView? pixels);
  texImage2D(..., ArrayBufferView? pixels);
  texSubImage2D(..., ArrayBufferView? pixels);

The new low level threading specification exposes a new type SharedArrayBuffer, and a new set of SharedArrayViews, see here:

https://docs.google.com/a/mozilla.com/document/d/1NDGA_gZJ7M7w1Bh8S0AoDyEqwDdRh4uSoTPSNn77PFk/edit#

The Shared* variants of the typed array buffer and views are exactly like the non-shared versions, except the new type is used to flag the JS engine that the objects can be shared across workers. For purposes of WebGL, this distinction has no extra functionality, except that the WebGL entry points need to recognize and accept the Shared* views like they accept the non-shared views. We need to have WebGL function entry points support for these Shared* variants.

Currently if one tries to pass e.g. a SharedUint32Array to GL.texImage2D pixels argument, it will throw an exception "TypeError: Argument 9 of WebGLRenderingContext.texImage2D does not implement interface ArrayBufferViewOrNull."

This bug is conceptually similar to https://bugzilla.mozilla.org/show_bug.cgi?id=1133859 , where the SIMD-JS entry points were expanded to accept Shared* views.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Assuming we don't ship SharedArrayBuffers on the main thread this is not useful until we get WebGL on a worker correct?

Comment 2

[Jukka Jylänki]

This is actually blocking me at the moment, as I'm working on getting a Unity demo working with pthreads. That demo will initially do blocking on the main thread (but later on probably not).

Comment 3

[Jukka Jylänki]

Attachment: sab-webidl.patch

I hacked together a first version that enables the Shared* variants in WebIDL and WebGL 1. It's not clean yet (introduces code duplication in WebGL context that I don't like) so not pushing for review yet, but a start at least.

Comment 4

[Jukka Jylänki]

I'm splitting the patch in two, first the WebIDL side, see the above bug, and once that lands, I'll push the rest here for review.

Comment 5

[Robert Goulet]

This is also blocking me.

Getting this error when trying to run webgl code in web workers (pthread support in Emscripten):

Argument 9 of WebGLRenderingContext.texImage2D does not implement interface ArrayBufferViewOrNull

Comment 6

[Jukka Jylänki]

Attachment: 0001-Bug-1147441-Add-SharedArrayBuffer-support-to-WebGL-a.patch

Adds support for WebGL 1 and WebGL 2 function entry points to take in SharedArrayBuffer and SharedTypedArray entry points.

Comment 7

[Jukka Jylänki]

The above attachment now contains a finished patch, which adds SAB compatibility, plus test for WebGL 1 and WebGL 2. Some notes:
   - The STA code is kind of temporary, and bug 1176214 is expected to remove those. The ArrayBuffer vs SharedArrayBuffer distinction will remain. The consensus with Lars in an earlier discussion was to go ahead with this patch and not wait for bug 1176214.
   - I don't know how to run test/webgl-mochitest/test_sab_with_webgl2.html in the mochitest environment except by manually launching the test in the built browser and setting the needed webgl2 prefs. In the suite it returns with a todo since the prefs are not set. Is this the desired way to test webgl2?

Comment 8

[Jukka Jylänki]

Sorry, raced with the comment!

One more comment:
   - since SAB is not a shipping feature yet, this is gated to only run in Nightly. However none of this code should need explicit #ifdef build checks, since the way the gating is done already is to limit users from creating SharedArrayBuffer and SharedTypedArray types. WebIDL supports SAB and STA unconditionally.

Comment 9

[Lars T Hansen [:lth]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #1)
> Assuming we don't ship SharedArrayBuffers on the main thread this is not
> useful until we get WebGL on a worker correct?

That assumption is not warranted at this time.  There are technical problems with /blocking/ the main thread - arguably browser implementation bugs but they may be unavoidable - but otherwise no technical or safety issues with exposing shared memory and atomics on the main thread per se.

In particular, there are no actual problems with exposing SharedArrayBuffer on the main thread, and it may make the shared memory feature unworkable if SharedArrayBuffer is not available on the main thread.  (Observe that you need to view the memory to access it, so if anything if you wanted to make shared memory unavailable on the main thread you would make it impossible to create views on shared memory on the main thread, not make it impossible to create shared memory.)

Comment 10

[Lars T Hansen [:lth]]

Comment on attachment 8666678 [details] [diff] [review]
0001-Bug-1147441-Add-SharedArrayBuffer-support-to-WebGL-a.patch

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

ArrayBufferViewOrSharedArrayBufferView is not a great name IMO.  The JS code uses "MaybeShared" as a suffix to denote the base type of the two view types, ie, ArrayBufferViewMaybeShared; it's at least a little shorter.

Note that ComputeArrayBufferViewOrSharedArrayBufferViewByteDataAndLengthAndType() removes the distinction between shared and unshared memory.  Naive accesses to the memory returned from this function may thus have data races, which is undefined behavior in C++, and TSan will be upset if that happens.  However, I think the patch is OK to land at the moment because bug 1176214 will clean that up, and in the mean time it is unlikely that there will be any races in practice (test cases would have to exist that explicitly create races; user code would not normally have any races because there would be a barrier sync following parallel rendering).  The general fix here is to use access primitives that are not subject to the C++ rule; those are landing as part of bug 1084248 and followups.

Comment 11

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8666678 [details] [diff] [review]
0001-Bug-1147441-Add-SharedArrayBuffer-support-to-WebGL-a.patch

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

Cool, just clean it up.

::: dom/canvas/WebGLContext.h
@@ +1664,4 @@
>  // Returns x rounded to the next highest multiple of y.
>  CheckedUint32 RoundedToNextMultipleOf(CheckedUint32 x, CheckedUint32 y);
>  
> +void ComputeArrayBufferViewOrSharedArrayBufferViewByteDataAndLengthAndType(const dom::ArrayBufferViewOrSharedArrayBufferView &view,

This function name is not OK. D:

@@ +1666,5 @@
>  
> +void ComputeArrayBufferViewOrSharedArrayBufferViewByteDataAndLengthAndType(const dom::ArrayBufferViewOrSharedArrayBufferView &view,
> +                                                                           void *&outData,
> +                                                                           uint32_t &outLength,
> +                                                                           js::Scalar::Type *outArrayType = 0);

Stars and ampersands to the left. Avoid optional and default args.

::: dom/canvas/WebGLContextBuffers.cpp
@@ +186,5 @@
> +// const dom::ArrayBuffer&
> +// const dom::SharedArrayBuffer&
> +// const dom::ArrayBufferView&
> +// const dom::SharedArrayBufferView&
> +template<typename BufferT>

These are OK for now, but we should really collapse this in the long run. We don't want to bloat our already bloated WebGL functions with 4x the produced binary code.

::: dom/canvas/WebGLContextGL.cpp
@@ +1385,5 @@
> +// collapse the SharedArrayBufferView and ArrayBufferView into one.
> +void ComputeArrayBufferViewOrSharedArrayBufferViewByteDataAndLengthAndType(const dom::ArrayBufferViewOrSharedArrayBufferView &view,
> +                                                                           void *&outData,
> +                                                                           uint32_t &outLength,
> +                                                                           js::Scalar::Type *outArrayType)

Stars and ampersands to the left, and `out_` is the prefix, not just `out`.
Also, out params must be pointers, preferrably const pointers. Newline after type and before function name. Prefer size_t to uint32_t for byte lengths.

void
ComputeLengthAndData(const dom::ArrayBufferViewOrSharedArrayBufferView& view,
                     void** const out_data, size_t* const out_length,
                     js::Scalar::Type* const out_type);

Use a junk var for out_type if it's unneeded at the callsite, instead of the complexity of an optional arg. Alternatively, have a different function which takes the view and gives back the js::Scalar::Type.

@@ +1392,5 @@
> +        const dom::ArrayBufferView& pixbuf = view.GetAsArrayBufferView();
> +        pixbuf.ComputeLengthAndData();
> +        outLength = pixbuf.Length();
> +        outData = pixbuf.Data();
> +        if (outArrayType) *outArrayType = JS_GetArrayBufferViewType(pixbuf.Obj());

Don't put this on the same line in general.

@@ +1482,5 @@
> +    const dom::ArrayBufferViewOrSharedArrayBufferView &view = pixels.Value();
> +    // Compute length and data.  Don't reenter after this point, lest the
> +    // precomputed go out of sync with the instant length/data.
> +    uint32_t dataByteLen;
> +    void *data;

Star to left.

::: dom/canvas/WebGLTextureUpload.cpp
@@ +132,5 @@
>          return mContext->ErrorInvalidOperation("compressedTexImage2D: internalFormat does not match the existing image");
>      }
>  
> +    uint32_t byteLength;
> +    void *data;

Star to left.

@@ +856,5 @@
>  
> +    void* data;
> +    uint32_t length;
> +    js::Scalar::Type jsArrayType;
> +    const dom::ArrayBufferViewOrSharedArrayBufferView& view = maybeView.Value();

const auto& should be fine here, too.

::: dom/canvas/test/webgl-mochitest/test_sab_with_webgl.html
@@ +53,5 @@
> +  var attribs = {
> +    antialias: false,
> +    depth: false,
> +  };
> +  gl = canvas.getContext('webgl', attribs);

It's still 'experimental-webgl' on FxOS, so please use that.

@@ +62,5 @@
> +  if (typeof SharedArrayBuffer === 'undefined') {
> +    todo(false, 'SharedArrayBuffer is unavailable.');
> +  }
> +  if (SharedFloat32Array === 'undefined') {
> +    todo(false, 'SharedFloat32Array is unavailable.');

Return early in these cases.

@@ +78,5 @@
> +  gl.linkProgram(prog);
> +
> +  var success = gl.getProgramParameter(prog, gl.LINK_STATUS);
> +  if (!success)
> +  {

{ on same line for single-line conditionals.

::: dom/canvas/test/webgl-mochitest/test_sab_with_webgl2.html
@@ +17,5 @@
> +  var attribs = {
> +    antialias: false,
> +    depth: false,
> +  };
> +  gl = canvas.getContext('webgl2', attribs);

This is the same except for the string here. Since it's not a failure to fail to create a webgl2 context here, just combine these tests.

@@ +26,5 @@
> +  if (typeof SharedArrayBuffer === 'undefined') {
> +    todo(false, 'SharedArrayBuffer is unavailable.');
> +  }
> +  if (SharedFloat32Array === 'undefined') {
> +    todo(false, 'SharedFloat32Array is unavailable.');

Return early in these cases.

Comment 12

[Jukka Jylänki]

(In reply to Lars T Hansen [:lth] from comment #10)
> Comment on attachment 8666678 [details] [diff] [review]
> 0001-Bug-1147441-Add-SharedArrayBuffer-support-to-WebGL-a.patch
> 
> Review of attachment 8666678 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ArrayBufferViewOrSharedArrayBufferView is not a great name IMO.

This name came automatically from the WebIDL binder. I could of course typedef it, but since it is temporary code, probably not worth(?)

Comment 13

[Lars T Hansen [:lth]]

(In reply to Jukka Jylänki from comment #12)
> (In reply to Lars T Hansen [:lth] from comment #10)
> > Comment on attachment 8666678 [details] [diff] [review]
> > 0001-Bug-1147441-Add-SharedArrayBuffer-support-to-WebGL-a.patch
> > 
> > Review of attachment 8666678 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ArrayBufferViewOrSharedArrayBufferView is not a great name IMO.
> 
> This name came automatically from the WebIDL binder. I could of course
> typedef it, but since it is temporary code, probably not worth(?)

If it's based on a common convention (because from the binder) then I have no objection.

Comment 14

[Jukka Jylänki]

Attachment: 0001-Bug-1147441-Add-SharedArrayBuffer-support-to-WebGL-a.patch

Comment 15

[Jukka Jylänki]

Addressed the previous review commits, could you take a quick peek again Jeff just to make sure it looks good?

Comment 16

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8668716 [details] [diff] [review]
0001-Bug-1147441-Add-SharedArrayBuffer-support-to-WebGL-a.patch

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

Is this really different from the previous patch? A bunch of things I commented on are unfixed. r-, please address (fix or comment on why not) my comments, both here and for my previous review.

::: dom/canvas/WebGL2Context.h
@@ +54,5 @@
>                             GLintptr readOffset, GLintptr writeOffset, GLsizeiptr size);
> +
> +private:
> +    template<typename BufferT>
> +    void GetBufferSubDataT(GLenum target, GLintptr offset, BufferT data);

`const BufferT&`

::: dom/canvas/WebGL2ContextBuffers.cpp
@@ +230,5 @@
> +    if (maybeData.IsNull())
> +        return ErrorInvalidValue("getBufferSubData: returnedData is null");
> +
> +    const dom::ArrayBuffer& data = maybeData.Value();
> +    GetBufferSubDataT<const dom::ArrayBuffer&>(target, offset, data);

You shouldn't need to explicitly mark these templates.

::: dom/canvas/WebGLContextGL.cpp
@@ +1385,5 @@
> +// collapse the SharedArrayBufferView and ArrayBufferView into one.
> +void ComputeArrayBufferViewOrSharedArrayBufferViewByteDataAndLengthAndType(const dom::ArrayBufferViewOrSharedArrayBufferView &view,
> +                                                                           void *&outData,
> +                                                                           uint32_t &outLength,
> +                                                                           js::Scalar::Type *outArrayType)

From my previous review:

Stars and ampersands to the left, and `out_` is the prefix, not just `out`.
Also, out params must be pointers, preferrably const pointers. Newline after type and before function name. Prefer size_t to uint32_t for byte lengths.

void
ComputeLengthAndData(const dom::ArrayBufferViewOrSharedArrayBufferView& view,
                     void** const out_data, size_t* const out_length,
                     js::Scalar::Type* const out_type);

Use a junk var for out_type if it's unneeded at the callsite, instead of the complexity of an optional arg. Alternatively, have a different function which takes the view and gives back the js::Scalar::Type.

::: dom/canvas/test/webgl-mochitest/test_sab_with_webgl.html
@@ +53,5 @@
> +  var attribs = {
> +    antialias: false,
> +    depth: false,
> +  };
> +  gl = canvas.getContext('webgl', attribs);

This needs to be experimental-webgl.

Comment 17

[Kelsey Gilbert [:jgilbert]]

It looks like the patches are the same size, so you probably just uploaded the unfixed version.

Comment 18

[Jukka Jylänki]

Attachment: Bug-1147441-SAB-in-WebGL-2.patch

Comment 19

[Jukka Jylänki]

Ops, very sorry! Indeed I uploaded the wrong file. How about this one? It should address all the review comments.

Comment 20

[Jukka Jylänki]

Attachment: Bug-1147441-SAB-in-WebGL-2.patch

There was a merge conflict with current mozilla-central with the previous patch, just with two #includes added next to each other in a same file, nothing else. Rebased to current mozilla-central tip. Not asking for review again, since this is identical to previous in all ways.

Comment 21

[Jukka Jylänki]

:lth ran a try build for the above patch, which is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d14f8da7ca04 . Everything is green, except for a OS X 10.7 debug run, which fails on what looks like an unrelated infra issue(?) "CalledProcessError: Command '['hg', 'log', '-r', 'd14f8da7ca04bd182849193828b7026eb3aff7d9', '--template', '{node}']' returned non-zero exit status 255", so looks like safe to ignore that.

Comment 22

[Carsten Book [:Tomcat]]

need dom peer review for the changes in WebGL2RenderingContext.webidl  and WebGLRenderingContext.webidl

Comment 23

[Jukka Jylänki]

:bz, could you take a look at this? I recall you reviewed my related work that this is based on (https://bugzilla.mozilla.org/show_bug.cgi?id=1168471)

Comment 24

[Boris Zbarsky [:bzbarsky]]

Which parts of this do you need me to review?  Just the idl changes, or the whole thing?

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8672608 [details] [diff] [review]
Bug-1147441-SAB-in-WebGL-2.patch

Ah, I guess yes, that's it.  r=me on the IDL changes.

Comment 26

[Jukka Jylänki]

Yeah, just the DOM changes. Sorry for not being clear! The WebGL operation itself was checked through by jgilbert.

Comment 27

[Jukka Jylänki]

Attachment: Bug-1147441-SAB-in-WebGL-2.patch

Added r=bzbarsky to the review list in patch comment, no other changes.

Comment 28

[Jukka Jylänki]

:Tomcat, how does this look now?

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/003b744576bd

Comment 30

[Nigel Babu [:nigelb]]

FYI: When you add a DOM peer, they need to be added on the commit as r=peername(so with more than one reviewer, "r=foo, r=bar") or else our hook won't catch it. I've made the change this time :)

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/003b744576bd

Comment 32

[Robert Goulet]

Awesome! Thanks for all this work! We currently use a custom build with this patch applied, so once this rolls out in the official Nightly we'll be able to verify this works as intended. Cheers!

Comment 33

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/bufferData
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/bufferSubData
https://developer.mozilla.org/en-US/docs/Web/API/WebGL2RenderingContext/getBufferSubData