Lars T Hansen [:lth] Reporter
	Description • 9 years ago

Feature request from Luke:

Right now the DOM TypedArray abstraction has been altered so that client code opts into using shared memory by calling LengthAllowShared() and DataAllowShared(); callers of Length() and Data() get 0 and NULL if the memory is shared, as if the TA were neutered.  This is safe but may result in some strange behavior and difficult-to-understand error messages.

A better approach might be for the WebIDL to change a little: the mapping of the various existing TypedArray IDL types could be changed so that they check the sharedness of the array argument and throw if the array is shared memory.  (The check is cheap.)  APIs that opt into shared memory would use new WebIDL types, eg, TypedArrayAllowShared, that do not perform this check.

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 9 years ago

> The check is cheap.

Is it either inlineable or doable as part of the function call the bindings already do for some other stuff?  We don't want to add another out of line function call on the webgl hot paths.

	Lars T Hansen [:lth] Reporter
	Comment 2 • 9 years ago

(In reply to Boris Zbarsky [:bz] from comment #1)
> > The check is cheap.
> 
> Is it either inlineable or doable as part of the function call the bindings
> already do for some other stuff?  We don't want to add another out of line
> function call on the webgl hot paths.

It should be inlinable.  There is a "buffer is shared memory" flag stored on the ObjectElements header, we inline the check of this flag in jitted code once we know we have a TypedArray.

https://dxr.mozilla.org/mozilla-central/source/js/src/vm/NativeObject.h#183
https://dxr.mozilla.org/mozilla-central/search?q=%2Bvar-ref%3Ajs%3A%3AObjectElements%3A%3AFlags%3A%3ASHARED_MEMORY

	Lars T Hansen [:lth] Reporter
	Comment 3 • 8 years ago

A tentative proposal is in the "DOM Companion spec" for the SharedArrayBuffer + Atomics proposal: https://tc39.github.io/ecmascript_sharedmem/dom_shmem.html#webgl

That proposal does not yet suggest what the predicate mechanism might look like in WebIDL; investigating that next.

	Lars T Hansen [:lth] Reporter
	Comment 4 • 8 years ago

The following seems to be a consensus position: https://www.w3.org/Bugs/Public/show_bug.cgi?id=29388#c2

Quoting Domenic from that comment, 

"It seems most natural to opt in on a per-argument basis. 

- ArrayBuffer -> (ArrayBuffer or SharedArrayBuffer)
- Int8Array -> [AllowShared] Int8Array, etc.
- ArrayBufferView -> [AllowShared] ArrayBufferView"

Domenic then makes the further suggestion that "There should then be some sort of requirement that specs which opt in to this have their processing models for the typed array/array buffer argument more well defined than they are currently. ..."

That would have a bearing on the WebIDL spec and the DOM Companion Spec for SAB but probably not (yet) on our DOM implementation, which I believe is safe already as a result of the fat pointers we use for buffer source memory.

	Lars T Hansen [:lth] Reporter
	Comment 5 • 8 years ago

It remains open what the behavior should be if the condition of the attribute is not met.  Suppose [AllowShared] is not present and an API is invoked on an ArrayBufferView on shared memory.  Does this mean:

(a) fail this case of the API and try the next one
(b) throw an exception right here

Consider XMLHttpRequest, which has a "send" method that can take any number of types and falls back on a variant that takes DOMString.  If We choose (a) above, an ArrayBufferView on shared memory will be converted to string and sent along - probably not desirable, but actually compatible with what that method does with "unknown" data.

	Anne (:annevk)
	Comment 6 • 8 years ago

I think throwing a TypeError would be better, since we know it's an error. That would also allow us to give more helpful information to developers.

	Lars T Hansen [:lth] Reporter
	Comment 7 • 8 years ago

Proof of concept, a little messy still but appears to work.  This one falls through to the next case (if there is one), though I agree it is better to just throw immediately.

	Lars T Hansen [:lth] Reporter
	Comment 10 • 8 years ago

bz, I'm wondering if I could have some feedback from you on this patch.  Not meant to be landable, but want some feedback before I continue in this direction.  Specifically, is this an acceptable direction, is there something that is obviously missing, etc.

Basically, I've added an [AllowShared] attribute that I want to apply optionally to ArrayBufferView.  (The attribute probably is not properly flagged as illegal if applied to other types; I don't know what the IDL custom is in regards to allowing / disallowing that.)

When [AllowShared] is present then the ArrayBufferView is allowed to map a SharedArrayBuffer, and so no code is emitted to check that it does not.  When the attribute is not present, code should always be emitted to check that ArrayBufferView memory is not shared.  The check is implemented by a reasonably-fast call to a JS API that extracts the sharedness bit, and is only performed once we've determined that we have an ArrayBufferView, so we will either commit to that type or fail (see comments from Anne above).

Included in the patch are some WebGL APIs that are suitably changed.  The test case (separate patch) shows that in the case of XMLHttpRequest, where the attribute has not been introduced, an error is thrown as desired.

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 8 years ago

The first question I have is what should happen in this situation:

  void foo([AllowShared] BufferSource arg);

I believe the attached implementation will _not_ allow shared array buffer views in that situation.  Should it?

	Lars T Hansen [:lth] Reporter
	Comment 12 • 8 years ago

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #11)

> The first question I have is what should happen in this situation:
> 
>   void foo([AllowShared] BufferSource arg);
> 
> I believe the attached implementation will _not_ allow shared array buffer
> views in that situation.  Should it?

There's no use case for that at the moment, to my knowledge, but I see your point: at a minimum, we might expect that [AllowShared]BufferSource = ([AllowShared]ArrayBufferView or ArrayBuffer).  That seems especially reasonable since ArrayBufferView is itself a union type and it is really [AllowShared]Int8Array etc that are primitive.

I guess in turn that argues that [AllowShared]T where T does not have a shared memory personality should be silently ignored, so that [AllowShared]ArrayBuffer == ArrayBuffer.

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 8 years ago

> There's no use case for that at the moment, to my knowledge

Well, mostly because the webgl spec has:

    void bufferData(GLenum target, ArrayBufferView data, GLenum usage);
    void bufferData(GLenum target, ArrayBuffer? data, GLenum usage);

instead of:

    void bufferData(GLenum target, BufferSource? data, GLenum usage);

which should be equivalent.  I guess my real question is whether we're sure we want an argument annotation as opposed to a new type for "array buffer view that can be a shared one".  The fact that ArrayBufferView is a union in the spec is a bit annoying; it means we'd actually need "shared" versions of all the typed array types and whatnot.

The other options are to punt on the union behavior and hope no one ever notices or to do some surgery on how unions work both in the IDL spec and in our implementation, because we'll need different conversion behavior for the same union depending on whether this extra flag is being used.

For the most part the IDL spec has been moving away from extended attributes on arguments, because of things like that....

	Lars T Hansen [:lth] Reporter
	Comment 14 • 8 years ago

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #13)
> 
> I guess my real question is whether we're sure
> we want an argument annotation as opposed to a new type for "array buffer
> view that can be a shared one".  The fact that ArrayBufferView is a union in
> the spec is a bit annoying; it means we'd actually need "shared" versions of
> all the typed array types and whatnot.

I see.  I don't have much of a stake in this per se.  The current spec (https://tc39.github.io/ecmascript_sharedmem/dom_shmem.html#webgl) introduces BufferDataSourceMaybeShared and ArrayBufferViewMaybeShared and then uses those, which is in line with what you write, though the definitions are maybe a little hand-wavy.  But then some of the big brains working on DOM suggested that an attribute is probably better (https://www.w3.org/Bugs/Public/show_bug.cgi?id=29388#c2), so I took that advice. 

Since WebIDL exposes Int8Array etc it seems somehow inevitable that we'll want to be able to distinguish those TA that map shared memory and those that don't.  Originally, we did have distinct names for the two classes of TA all the way into JS, but on the JS side the committee did not want that...  That doesn't have to constrain DOM and IMO DOM would be better off by being more explicit here.

> The other options are to punt on the union behavior and hope no one ever
> notices or to do some surgery on how unions work both in the IDL spec and in
> our implementation, because we'll need different conversion behavior for the
> same union depending on whether this extra flag is being used.
> 
> For the most part the IDL spec has been moving away from extended attributes
> on arguments, because of things like that....

I see the appeal of that...

The alternative to the extended attribute is, in summary:

  new type SharedArrayBuffer, imported from JS

  new types SharedInt8Array etc -- typedarrays that map shared memory, on the JS side these are
        TypedArray types whose buffer instanceof SharedArrayBuffer

  new type SharedDataViev -- dataview that maps shared memory, on the JS side a DataView whose
        buffer instanceof SharedArrayBuffer

  new type ArrayBufferViewMaybeShared -- typedarrays and dataview that may or may not map shared memory,
        ie a union of (Int8Array, ..., SharedInt8Array, ..., DataView, SharedDataView)

  new type BufferSourceMaybeShared -- a union of ArrayBufferViewMaybeShared, ArrayBuffer, 
        and SharedArrayBuffer

Then we switch to ArrayBufferViewMaybeShared and BufferSourceMaybeShared in DOM APIs where appropriate, currently only WebGL and WebGL2 APIs.

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 8 years ago

> That doesn't have to constrain DOM and IMO DOM would be better off by being more explicit here.

Yes, I agree.

> new type SharedArrayBuffer

SharedArrayBuffer, or "MaybeSharedArrayBuffer"?  That is, are there any use cases for requiring that the passed-in thing is a SharedArrayBuffer as opposed to either requiring that it be unshared or allowing both?  I guess it's simplest to have SharedArrayBuffer and let people use unions as needed...

Anne, thoughts?

	Domenic Denicola
	Comment 16 • 8 years ago

I don't have strong opinions on the representation here, partially because I am still struggling to understand the problem with union types and extended attributes. I know in the past that has led to things like USVString but I never fully understood the story there.

Is the problem mainly that [AllowShared] BufferSource would expand to include [AllowShared] ArrayBuffer, and that's weird?

But isn't that kind of what we want---with the additional rule that when converting a JS ArrayBuffer to a Web IDL ArrayBuffer, it throws if IsSharedArrayBuffer(jsObj) is true and [AllowShared] is not specified?

---

I'm also minorly concerned about introducing a break between Web IDL Int8Array/ArrayBuffer and JS Int8Array/ArrayBuffer. My understanding is that we're on a course where not all JS Int8Arrays can be converted to Web IDL Int8Arrays; they can instead only be converted if [AllowShared] is present, or can only be converted to MaybeSharedInt8Arrays. That seems a bit sad. But I guess the alternative is updating every spec that currently takes Int8Array to instead take UnsharedInt8Array, which is probably not worth it...

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 8 years ago

> partially because I am still struggling to understand the problem with union types and extended attributes.

The problem is that the extended attribute hangs the argument, not the view type that the conversion is happening to.  So if you want it to work for unions, you have to thread that information down through the union's conversion algorithm (in the spec and in implementation).  This is a pain all around.

	Anne (:annevk)
	Comment 18 • 8 years ago

It seems w3.org is down at the moment so I can't figure out exactly what https://www.w3.org/Bugs/Public/show_bug.cgi?id=28798 says, but we should take that issue into account as well if we start changing how this works.

In particular, reference-vs-copy semantics are an important consideration and would create even more types down the road if not considered now. Here is a solution that I think makes sense:

* Int8Array, ArrayBuffer, etc. all act as copying at the boundary and throw for shared buffers. The specification algorithm ends up with a simple byte sequence and has no worries.
* UnsafeInt8Array, UnsafeArrayBuffer, etc. all act as reference and allow shared buffers. The specification algorithm ends up with the JavaScript object and needs to handle it very carefully. Reviewers will hopefully be alert to that fact due to the Unsafe* prefix.

This might require changes to a couple of APIs to use Unsafe* instead (and manually forbid SharedArrayBuffer) if they need by reference semantics, but there should not be many of those.

	Lars T Hansen [:lth] Reporter
	Comment 19 • 8 years ago

(At the risk of bikeshedding prematurely, "Unsafe" is not a good flag since it communicates only fear, not the reason why we should be afraid.  Do consider "MaybeShared" as a more informative alternative, or "MaybeSharedByRef".)

	Lars T Hansen [:lth] Reporter
	Comment 20 • 8 years ago

Supposedly this is about to be resolved:

https://github.com/tc39/ecmascript_sharedmem/issues/168#issue-207604011
https://github.com/heycam/webidl/pull/286

The resolution appears to be that we've come full circle and that there will be an [AllowShared] attribute that can be brought to bear on this, with sufficient WebIDL changes to avoid pitfalls discussed above.

	Lars T Hansen [:lth] Reporter
	Comment 21 • 7 years ago

(Not working on this.)