On Tuesday, December 15, 2015 at 5:41:39 PM UTC-8, Kenneth Russell wrote:
On Tue, Dec 15, 2015 at 4:03 PM, Ben Smith
<bi...@chromium.org> wrote:
On Tuesday, December 15, 2015 at 3:28:36 PM UTC-8, Domenic Denicola wrote:From: blin...@chromium.org [mailto:blin...@chromium.org] On Behalf Of Ben Smith
> We may allow some methods to do this, but I'd like that list to be opt-in instead of opt-out.
This seems like an important question for the relevant standards, probably most pressingly Web IDL. Have you solicited opinions from the editors of those standards?
Not yet, thanks for the suggestion.
My initial reaction is that this seems strange and incorrect, as Web IDL already specifies that all passed-in typed arrays are copied into a new backing store before they are operated on by spec algorithms, so there is reason to disallow SharedArrayBuffer-backed typed arrays. (Note: sometimes the passed-in typed array is neutered before being used, which allows the implementation to make the unobservable optimization of omitting the spec's copy.)
I assume you mean there is "no" reason to disallow? This may be specified (I found
this, is that what you mean?), but taking a quick look at a few methods that take ArrayBuffers in Blink, they seem to be reading directly from the buffer. It makes some sense: they wouldn't need to copy data into a new backing store if they don't use the ArrayBuffer contents after control is returned to JavaScript, since the contents can't be modified. But as soon as you have a SharedArrayBuffer, this is an unsafe assumption to make since the contents could be modified by another thread.
Yes, most APIs taking ArrayBuffer or ArrayBufferView read directly from the incoming buffer; this is deliberate, for the reasons you've pointed out. SharedArrayBuffer does present a new concurrency problem for these APIs.
Adding an implicit check to the binding generator doesn't sound like a desirable approach since:
- SharedArrayBuffer support is still behind a flag
Right, but whatever method we use to prevent using SAB views will have to be added before the flag is default-enabled anyway.
- Presumably, we'll have to think through the implications of SABs for all of these APIs before it's turned on by default
- It'll add undesired overhead to all of these API calls in the interim
OK, so you're concerned about that APIs that will end up supporting SAB views (so they ultimately won't need the check) but will currently not be opted-in, so they'll have an additional check. Are there any good perf tests I can run to measure the effect of this additional check?
Rather than adding an implicit check, I'd like to suggest a couple of other possibilities:
- Adding an extended attribute to the binding generator which would add the new checks for the specific APIs where they are wanted (and make them no-ops if SharedArrayBuffers aren't actually turned on). (I realize this doesn't allow your opt-in approach, but again, this functionality is still behind a flag.)
Well, SharedArrayBuffers are a runtime flag, so they can't be disabled in the generated binding code. Are you saying to check the SharedArrayBuffer flag rather than the isShared() flag on the ArrayBuffer object? The isShared() check should be nearly as cheap (it's just an enum comparison) though it does require some pointer indirection. If we found that it was slow, we could cache the isShared flag higher up -- it can't ever change after being set initially.
- Adding code to the C++ methods themselves which would do these checks and raise an exception if a SAB, or an ArrayBufferView backed by an SAB, is passed in (and make these checks no-ops if SharedArrayBuffers aren't turned on)
- Adding code to the C++ methods themselves which would copy the data out of the SAB (which would become a no-op if SABs aren't turned on). This obviously wouldn't work for APIs that pass in an ArrayBuffer or ArrayBufferView to be filled.
Why would handling it in the C++ methods be better? It seems like it would be more likely to be forgotten and would ultimately end up doing the same check.