SharedArrayBuffer looses wasm_memory flag on transfer via postmessage |
||||
Issue descriptionI discovered this while debugging a bug around wasm threads. With wasm thread support enabled, WebAssembly.Memory allocates a shared array buffer as backing store for the memory. This buffer is tagged as wasm memory (is_wasm_memory getter) and as shared. When we transfer this via postMessage, the buffer is actually shared via the embedder. Blink, in my case, does not know about the is_wasm_memory flag and will create a new shared array buffer object in the other worker that is tagged as shared but has lost the is_wasm_memory flag. As a consequence, some DCHECK will fail. To fix this, we could add the wasm memory flag on the embedder side so that we can recreate the corresponding state for the v8 object. This would require changes to the embedder api. Another fix would be to use the wasm allocation tracker to identify wasm memory objects when they are passed into a wasm instantiate etc.. This would require a single wasm engine across workers, which is already planned but not yet ready. For now, I will change the DCHECK to allow non-wasm memory objects if they are shared.
,
Apr 25 2018
I think this is something we should get sorted out before we enable Wasm threads by default. If we can fix this without changing the embedder API, I think that would be preferable. The Wasm memory tracker has the source of truth for whether ArrayBuffers are a Wasm memory, so I like the idea of reconstructing this information when the buffer comes back to V8. Is work towards sharing the Wasm engine going on now, or is it still planned for the future?
,
Apr 25 2018
We are getting close to sharing the WasmEngine between all isolates within a process and it is actively being worked on. We discussed a solution to this where we have the knowledge centralized in the WasmEngine, which of course works when the WasmEngine is shared between the postMessage()er and the postMessage()ee. However, if the two don't share a WASM engine, we'll need another solution. WDYT? Also, it is unclear to me how SharedArrayBuffer's backing store is deallocated, but it's a related problem that we'll need to free the guard regions at that point.
,
Apr 25 2018
The only case where they wouldn't be able to share a WASM engine is when they are in different processes, right? Can SABs be shared between processes? If they don't share a WASM engine, we'll need some way to do shared ownership of SABs. Blink basically does something like this. It's a source of a lot of complexity, but if we can make that work it's probably better to reuse that than basically duplicate what Blink already does. As far as when ArrayBuffers and SABs get their backing store deallocated, there are two ways this happens. The first is in JSArrayBuffer::FreeBackingStore (https://cs.chromium.org/chromium/src/v8/src/objects.cc?type=cs&q=ArrayBuffer::FreeBackingStore&sq=package:chromium&l=19186), which normally is called by the ArrayBufferTracker. The second way happens in Blink for externalized buffers. When the buffer is externalized, we remove it from the WasmMemoryTracker and transfer all the information about the allocation to Blink as an ArrayBufferContents: https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/wtf/typed_arrays/array_buffer_contents.h?q=ArrayBufferContents&dr=CSs&l=41 The ArrayBufferContents gets a DataHandle associated with it, which frees the backing store in its destructor.
,
Apr 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/0380b69af5ddfb13a2dec574be2b232175d3288a commit 0380b69af5ddfb13a2dec574be2b232175d3288a Author: Stephan Herhut <herhut@chromium.org> Date: Thu Apr 26 07:36:56 2018 Allow use of ordinary shared array buffer in wasm threads For wasm memory buffers, we normally require the memory to be allocated via WebAssembly.Memory, which will set a is_wasm_memory flag and register the memory with the wasm allocation tracker. This CL weakens that requirement in a DCHECK to allow for running experimental threaded applications even though the is_wasm_memory flag is not currently propagated via postMessage. Bug: chromium:836800 Change-Id: I4613b8651423307ce4cd466c0df28fc43244ec4f Reviewed-on: https://chromium-review.googlesource.com/1027813 Commit-Queue: Stephan Herhut <herhut@chromium.org> Reviewed-by: Ben Titzer <titzer@chromium.org> Reviewed-by: Eric Holk <eholk@chromium.org> Cr-Commit-Position: refs/heads/master@{#52801} [modify] https://crrev.com/0380b69af5ddfb13a2dec574be2b232175d3288a/src/wasm/module-compiler.cc
,
Apr 27 2018
Sorry, my language was sloppy and probably caused some confusion. I have read up on postMessage terminology now :) When creating new workers to implement threads, the code passes the shared buffer that is used as wasm memory via postmessage to the new worker. It is not on the transfer list, so ownership is not transferred but the buffer gets shared. See allocateUnusedWorkers in the emscripten js output. The problem arises as this sharing still happens via the embedder and we loose information when the buffer is passed from the main thread to the worker.
,
Aug 23
,
Aug 23
,
Aug 23
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/524215be1a29a23fcb44eee31ae98f1cac0c7746 commit 524215be1a29a23fcb44eee31ae98f1cac0c7746 Author: Stephan Herhut <herhut@chromium.org> Date: Thu Aug 23 15:47:17 2018 Use new arraybuffer deleter interface in d8 With this cl we start using the custom deleter to free externalized array buffers. This also allows us to keep wasm memories registered with the wasm memory tracker and thereby to propagate that a memory is wasm allocated over postMessage calls. Bug: v8:8073 , chromium:836800 Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: I57e3ea44d9c6633ada7996677dd1de4da810ab64 Reviewed-on: https://chromium-review.googlesource.com/1186681 Reviewed-by: Michael Starzinger <mstarzinger@chromium.org> Commit-Queue: Stephan Herhut <herhut@chromium.org> Cr-Commit-Position: refs/heads/master@{#55361} [modify] https://crrev.com/524215be1a29a23fcb44eee31ae98f1cac0c7746/src/api.cc [modify] https://crrev.com/524215be1a29a23fcb44eee31ae98f1cac0c7746/src/d8.cc [modify] https://crrev.com/524215be1a29a23fcb44eee31ae98f1cac0c7746/src/d8.h [modify] https://crrev.com/524215be1a29a23fcb44eee31ae98f1cac0c7746/src/wasm/module-compiler.cc
,
Aug 23
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/eb1eaf636188488514315e75e55cc4e604553bc5 commit eb1eaf636188488514315e75e55cc4e604553bc5 Author: Michael Achenbach <machenbach@chromium.org> Date: Thu Aug 23 18:29:23 2018 Revert "Use new arraybuffer deleter interface in d8" This reverts commit 524215be1a29a23fcb44eee31ae98f1cac0c7746. Reason for revert: Breaks cfi: https://ci.chromium.org/p/v8/builders/luci.v8.ci/V8%20Linux64%20-%20cfi/16422 Original change's description: > Use new arraybuffer deleter interface in d8 > > With this cl we start using the custom deleter to free externalized > array buffers. This also allows us to keep wasm memories registered > with the wasm memory tracker and thereby to propagate that a memory > is wasm allocated over postMessage calls. > > Bug: v8:8073 , chromium:836800 > Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng > Change-Id: I57e3ea44d9c6633ada7996677dd1de4da810ab64 > Reviewed-on: https://chromium-review.googlesource.com/1186681 > Reviewed-by: Michael Starzinger <mstarzinger@chromium.org> > Commit-Queue: Stephan Herhut <herhut@chromium.org> > Cr-Commit-Position: refs/heads/master@{#55361} TBR=mstarzinger@chromium.org,herhut@chromium.org Change-Id: I64c4e76d8d68bad8df4ba3297c099b9b44eabc7c No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: v8:8073 , chromium:836800 Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Reviewed-on: https://chromium-review.googlesource.com/1187241 Reviewed-by: Michael Achenbach <machenbach@chromium.org> Commit-Queue: Michael Achenbach <machenbach@chromium.org> Cr-Commit-Position: refs/heads/master@{#55366} [modify] https://crrev.com/eb1eaf636188488514315e75e55cc4e604553bc5/src/api.cc [modify] https://crrev.com/eb1eaf636188488514315e75e55cc4e604553bc5/src/d8.cc [modify] https://crrev.com/eb1eaf636188488514315e75e55cc4e604553bc5/src/d8.h [modify] https://crrev.com/eb1eaf636188488514315e75e55cc4e604553bc5/src/wasm/module-compiler.cc
,
Aug 24
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/89bea4c050a3b73a21e7380cf805825cd52ab3c8 commit 89bea4c050a3b73a21e7380cf805825cd52ab3c8 Author: Stephan Herhut <herhut@chromium.org> Date: Fri Aug 24 10:37:09 2018 Reland "Use new arraybuffer deleter interface in d8" This is a reland of 524215be1a29a23fcb44eee31ae98f1cac0c7746 Original change's description: > Use new arraybuffer deleter interface in d8 > > With this cl we start using the custom deleter to free externalized > array buffers. This also allows us to keep wasm memories registered > with the wasm memory tracker and thereby to propagate that a memory > is wasm allocated over postMessage calls. > > Bug: v8:8073 , chromium:836800 > Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng > Change-Id: I57e3ea44d9c6633ada7996677dd1de4da810ab64 > Reviewed-on: https://chromium-review.googlesource.com/1186681 > Reviewed-by: Michael Starzinger <mstarzinger@chromium.org> > Commit-Queue: Stephan Herhut <herhut@chromium.org> > Cr-Commit-Position: refs/heads/master@{#55361} Bug: v8:8073 , chromium:836800 Change-Id: Ia3c057ced496363cfdd07eed16ed1d0c7a3f3084 Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Reviewed-on: https://chromium-review.googlesource.com/1188222 Reviewed-by: Michael Starzinger <mstarzinger@chromium.org> Commit-Queue: Stephan Herhut <herhut@chromium.org> Cr-Commit-Position: refs/heads/master@{#55389} [modify] https://crrev.com/89bea4c050a3b73a21e7380cf805825cd52ab3c8/src/api.cc [modify] https://crrev.com/89bea4c050a3b73a21e7380cf805825cd52ab3c8/src/d8.cc [modify] https://crrev.com/89bea4c050a3b73a21e7380cf805825cd52ab3c8/src/d8.h [modify] https://crrev.com/89bea4c050a3b73a21e7380cf805825cd52ab3c8/src/wasm/module-compiler.cc
,
Dec 4
|
||||
►
Sign in to add a comment |
||||
Comment 1 by gdeepti@chromium.org
, Apr 25 2018