Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

Is it appropriate to "transfer" SABs? #98

Closed
lars-t-hansen opened this issue May 3, 2016 · 17 comments
Closed

Is it appropriate to "transfer" SABs? #98

lars-t-hansen opened this issue May 3, 2016 · 17 comments
Labels

Comments

@lars-t-hansen
Copy link
Collaborator

@domenic @annevk

This came up in a discussion with @dherman: Is it appropriate for SABs to be sent from one agent to another with the structured clone transfer mechanism (as we have now) or without it? It turns out he and I have different intuitions about what is entailed by "transfer". To me it it entails "don't deep copy". To Dave it entails "detach the buffer, don't share it". Clearly, for ArrayBuffer both intuitions hold.

I don't know precisely how we came to this point where a SAB must be in the transfer list. Judging from our Mercurial log I inherited the mechanism with the prototype code I took over from @sstangl, and given my intuition about structured clone I always thought it was a reasonable choice. I don't know if there's any deeper background, eg, a design that comes from PNaCl for example.

Another justification that comes up from time to time is that having to add a SAB to the transfer map makes it easier to guarantee that a SAB is never shared accidentally. I happen to think that's a benefit, but not if it runs counter to some generally agreed-upon aspect of transfering. (Nobody's raised any flags before now.)

@annevk
Copy link
Member

annevk commented May 3, 2016

The alternative would be to use cloning?

If you squint you could maybe see Blob as a readonly variant of SharedArrayBuffer (while cloning a Blob you also copy the pointer to the underlying resource). From that perspective not using transfer semantics might make sense.

It's true that when formalizing the clone and transfer semantics in https://html.spec.whatwg.org/multipage/infrastructure.html#safe-passing-of-structured-data [[Detached]] being a slot of transferable objects was seen as a primitive so @dherman might be right on the money here.

@domenic
Copy link
Member

domenic commented May 3, 2016

I tend to agree that the primary semantic of transferring is that the original object is no longer usable.

@lars-t-hansen
Copy link
Collaborator Author

Looking at our code a little more, the original design might have been influenced by how easy it is to implement, since the pointer to the shared memory (a SharedArrayRawBuffer in our code) is put into the transfer map in the same way as a pointer to an ArrayBuffer's buffer memory would be, the only difference is that the SAB is not detached on the sending side.

From that point of view, thinking of passing a SAB as a transfer is highly appropriate, and detaching is a separate operation that is not implied by transfering. (Just observing, not judging.)

@annevk
Copy link
Member

annevk commented May 3, 2016

On the observation front:

  1. We discussed earlier how even transferred ArrayBuffer would need to be specified as if it was cloned due to various CPU architectures. You argued we could not require that it had shared memory. So while an implementation might do that, from a specification perspective that argument does not hold much value.
  2. The fact that Blob does not use that approach, while having similar shared-data semantics, seems weird.

@lars-t-hansen
Copy link
Collaborator Author

Yes, there's just no way you can really require non-copying in the absence of observable concurrent side effects or certain leaky abstractions (eg revealing memory addresses might make it hard to copy but probably not impossible).

(For new listeners the issue at hand is browsers with process separation; if an ArrayBuffer is transfered from one agent to another but the agents are in two separate processes, the implementation will most likely wish to copy the memory and not implement cross-process shared memory.)

@jfbastien
Copy link
Contributor

NaCl is in a separate process, so it copies things over, JS just does postMessage and NaCl does something similar to copy_from_user into the JS process. A similarity is in how NaCl supports page protections, especially W^X protections: it's really hard to "transfer" the state from one to another unless you're sure that all user threads have quiesced, usually through a syscall.

It seems like something similar applies to SAB if transfer implies original users can't access the SAB anymore: all users have to call in before this can be completed.

@erights
Copy link

erights commented May 3, 2016

I would think that two agents, separated by a boundary over which they cannot coherently share mapped pages, would therefore be in two separate agent clusters. Indeed, I thought the main motivation for defining agent clusters is to surface this implementation issue when coherent mapped sharing is impossible.

Note that on most operating systems, processes can coherently share mapped pages. So process separation itself should not necessarily force agent cluster separation. OTOH, two agents running in two separate machines without coherent shared memory (ironically, let's say, in a cluster) would necessarily be in two different agent clusters. This issue has also shown up within a "machine" in some historic multicore designs, like the Transputer or the IBM Cell processor. As the number of cores on a chip explodes, we should not expect coherent sharing to scale.

@erights
Copy link

erights commented May 3, 2016

When I raised the issue of detaching SABs in the hope of safely terminating agents without terminating the whole agent cluster, at #55 (comment) @lukewagner said:

The first two bullets would I believe have pretty major implementation/performance/security hazards. It means that, between any two instructions (in JIT code, in C++ code iterating over a typed array view, etc), a SAB could have become detached. Just the possibility of an AB getting detached synchronously by reentering JS which postMessage()d caused a number of browser security bugs a few years ago. It seems like implementations would be forced into various complicated schemes to avoid hurting performance too much and this would require audits of all AB/TA-accessing code to detect this new kind of racy detachment.

Does this hazard apply with just as much force to detachment-on-transfer? (I would expect it does)

@lars-t-hansen
Copy link
Collaborator Author

lars-t-hansen commented May 3, 2016

Before this discussion veers off course irretrievably, let me try to rein it in:

The only thing I want resolved in this bug is whether, when a SAB is sent from one agent to another in an HTML embedding, one needs to write postMessage(sab, [sab]) or whether one should simply write postMessage(sab). This is really an HTML / StructuredClone spec issue, and it is only a question of surface syntax, for lack of a better phrase. There are no deep semantic implications.

In particular, this discussion has nothing to do with agent clusters, or whether something can or cannot be copied or anything like that, and I apologize for nibbling at @annevk's bait in this regard. I can only blame myself.

Please open new bugs for all other questions, or followup in existing bugs.

EDIT: corrected embarrassing typo.

@domenic
Copy link
Member

domenic commented Jul 20, 2016

To try to close this loop. I think postMessage(sab, [sab]) should be the syntax if sab will be detached (in the realm doing the transfer), whereas postMessage(sab) should be the syntax if sab will not be detached.

@annevk
Copy link
Member

annevk commented Jul 21, 2016

I tend to agree with @domenic. @lars-t-hansen made an argument in #98 (comment) based on code, but presumably the code for Blob objects is similar and lives on the other side.

@lars-t-hansen
Copy link
Collaborator Author

And I think I agree with both @annevk and @domenic.

@binji, do you have any opinions about this? Personally I'm happy to defer to the folk who are closer to HTML than me, but good if we are all known to agree before we jump.

(This is outside the ES spec proper in any case but me or Anne will need to shepherd it through the appropriate channels.)

@binji
Copy link

binji commented Jul 21, 2016

Well, as @lars-t-hansen mentioned, the code is nicer if the SAB is in the transfer list. The behaviors are very similar, when an ArrayBuffer (shared or otherwise) is not in the transfer list, the data is copied, so that code doesn't have to change. When it is in the transfer list, in both cases only the pointer is copied. The only difference is whether the original is detached or not.

I took a look at the way it's done for blobs, and it is similar. They just store a list of blob indexes and blob handles, then extract that. The difference is that we'll want to share the logic for transferring ArrayBuffers and sharing SharedArrayBuffers. This isn't an issue with blobs, since they can't be transferred. Not a big deal, but the code will be a little nastier, I suspect.

I guess that's another question: if we make this change, I assume SharedArrayBuffers will not be allowed to be transferred since they can't be detached. Is that right? This also means that if you did want to send a copy of a SAB (for some reason), you'd have to copy the data to an ArrayBuffer, send it, then copy the new ArrayBuffer to a new SAB. Probably not too much of a problem, as it's hard to imagine why you'd want to copy a SAB in the first place.

@annevk
Copy link
Member

annevk commented Jul 21, 2016

If copying became desired, we could add cloning support to SAB, no? And yeah, you cannot transfer objects that cannot be detached at the moment.

@domenic
Copy link
Member

domenic commented Jul 21, 2016

@annevk I don't think so, because by deciding that postMessage(sab) should be the syntax, we've defined cloning to be a "shallow clone". That is, it clones the object, but the object only consists of a pointer to backing data which is (usually) not copied.

@annevk
Copy link
Member

annevk commented Jul 22, 2016

I meant with a new API. Same as we would have to do for blobs.

@lars-t-hansen
Copy link
Collaborator Author

This matter was resolved a while back: the SAB should not be in the transfer list, and a DataCloneError should be thrown if it is. The change has been recorded in the canonical DOM companion spec, https://tc39.github.io/ecmascript_sharedmem/dom_shmem.html#postMessage.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

6 participants