Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add [AllowShared] to allow accepting SharedArrayBuffers #353

Merged
merged 1 commit into from May 3, 2017

Conversation

domenic
Copy link
Member

@domenic domenic commented Apr 28, 2017

Previous to this commit, any API that accepted any of the buffer source types would also accept a SharedArrayBuffer. This was not the approach we want; instead, we want APIs to explicitly opt-in. Enabled by previous work creating annotated types, this adds the new [AllowShared] extended attribute to allow APIs to opt in to accepting SharedArrayBuffers.

While here, this removes an unimplemented check for detached array buffers when converting from ES values to IDL types, fixing #352.


Preview | Diff

Previous to this commit, any API that accepted any of the buffer source types would also accept a SharedArrayBuffer. This was not the approach we want; instead, we want APIs to explicitly opt-in. Enabled by previous work creating annotated types, this adds the new [AllowShared] extended attribute to allow APIs to opt in to accepting SharedArrayBuffers.

While here, this removes an unimplemented check for detached array buffers when converting from ES values to IDL types, fixing #352.
@annevk
Copy link
Member

annevk commented Apr 30, 2017

So this is primarily for WebGL at this point right? Do we have some follow-up filed there?

@domenic
Copy link
Member Author

domenic commented May 1, 2017

Yes, although I am told it might expand over time, e.g. for web audio or similar.

We don't have a follow-up filed on Web GL, because to my knowledge their specs are developed in private with no public issue tracker or "editor's draft". I am not sure what the procedure will be. Maybe we'll need to maintain a moneypatch somewhere.

I'll try to poke tc39/proposal-ecmascript-sharedmem#168 about that issue.

@bzbarsky
Copy link
Collaborator

bzbarsky commented May 1, 2017

The WebGL issue tracker is at https://github.com/KhronosGroup/WebGL/issues

@domenic
Copy link
Member Author

domenic commented May 1, 2017

Ah OK yeah, I just found that myself; clearly my info was out of date.

@bzbarsky
Copy link
Collaborator

bzbarsky commented May 1, 2017

And https://www.khronos.org/registry/webgl/specs/latest/ links to the editor's drafts of WebGL1 and WebGL2, fwiw.

domenic added a commit to domenic/WebGL that referenced this pull request May 1, 2017
This annotates all ArrayBuffer or ArrayBufferView types with [AllowShared], per the proto-specification at https://tc39.github.io/ecmascript_sharedmem/dom_shmem.html#webgl, extended to also modify the Web GL 2.0 entrypoints. This allows the backing data to be a SharedArrayBuffer, instead of only non-shared ArrayBuffers, per whatwg/webidl#353.

This also makes some related editorial changes, removing the redundant BufferDataSource typedef in favor of the defined-in-Web IDL BufferSource type, and consolidating two overloads of bufferData (one accepting ArrayBuffer? and the other accepting ArrayBufferView) into a single overload accepting BufferSource?.
Copy link
Collaborator

@tobie tobie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

A type that is not a [=buffer source type=] must not be
[=extended attributes associated with|associated with=] the [{{AllowShared}}] extended attribute.

See the rules for converting ECMAScript values to IDL [=buffer source types=] in
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I admit I had to look "entails" up. Maybe instead write:

[…] for the specific requirements that using [{{AllowShared}}] creates.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already used for every other extended attribute, so I'd rather be consistent.

@domenic
Copy link
Member Author

domenic commented May 2, 2017

Does anyone else want to review before we merge? Ideally speak up within the next day-ish.

@domenic domenic merged commit c59bdcb into master May 3, 2017
@domenic domenic deleted the allowshared branch May 3, 2017 19:29
kenrussell pushed a commit to KhronosGroup/WebGL that referenced this pull request May 10, 2017
* Support SharedArrayBuffers

This annotates all ArrayBuffer or ArrayBufferView types with [AllowShared], per the proto-specification at https://tc39.github.io/ecmascript_sharedmem/dom_shmem.html#webgl, extended to also modify the Web GL 2.0 entrypoints. This allows the backing data to be a SharedArrayBuffer, instead of only non-shared ArrayBuffers, per whatwg/webidl#353.

This also makes some related editorial changes, removing the redundant BufferDataSource typedef in favor of the defined-in-Web IDL BufferSource type, and consolidating two overloads of bufferData (one accepting ArrayBuffer? and the other accepting ArrayBufferView) into a single overload accepting BufferSource?.

* Also allow SharedArrayBuffer in the -List typdefs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants