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

syscall/js: huge copy overhead when passing byte slices over certain Go/JS boundaries #26193

Closed
leidegre opened this issue Jul 3, 2018 · 21 comments
Milestone

Comments

@leidegre
Copy link

leidegre commented Jul 3, 2018

This related to my effort to get Go WASM/JS to work with a WebWorker process.

What version of Go are you using (go version)?

go version devel +0e0cd70ecf Tue Jul 3 04:16:23 2018 +0000 windows/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

set GOARCH=wasm
set GOBIN=
set GOCACHE=C:\Users\leidegre\AppData\Local\go-build
set GOEXE=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=js
set GOPATH=C:\Users\leidegre\go
set GOPROXY=
set GORACE=
set GOROOT=C:\Users\leidegre\Source\go
set GOTMPDIR=
set GOTOOLDIR=C:\Users\leidegre\Source\go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set CXX=g++
set CGO_ENABLED=0
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-fPIC -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=C:\Users\leidegre\AppData\Local\Temp\go-build097448038=/tmp/go-build -gno-record-gcc-switches
set VGOMODROOT=

What did you do?

Trying to pass an nil []byte slice from a WebWorker process to a web application.

What did you expect to see?

That gigabytes of memory is not allocated.

What did you see instead?

Gigabytes of memory is being allocated on each postMessage call. This crashes both Chrome and Firefox render process. Haven't tested Edge.

Additional findings

as soon as you console.log or pass a slice through a boundary context, for example postMessage there's copying associated with the ArrayBuffer. For some reason, the gigabyte ArrayBuffer (Go linear memory) gets copied as well and the heap just explodes typically grinding the web page to a halt or crashing the page.

I was able to work around this issue by passing the slice through this helper function first.

[worker.js]

function slice({ byteOffset, byteLength, buffer }) {
  return buffer.slice(byteOffset, byteOffset + byteLength)
}

[worker.go]

var x []byte
x = append(x, 2)
x = append(x, 3)
x = append(x, 5)
y := js.TypedArrayOf(x)
z := global.Call("slice", y)
y.Release()

global.Call("postMessage", z) // from WebWorker postMessage

Note that problem occur even if the slice is empty or nil. If worker.js, i.e. the WebWorker process does console.log at any time on any object that is backed by the ArrayBuffer you'll see crazy heap explosion and subsequent crashes. Not sure what to do about this but I thought I'd share my findings here. It's mostly a side effect of how things are not something Go/WASM is doing wrong but maybe this can be improved somehow? Passing values between Go/JS seems like a natural thing to do.

@leidegre leidegre changed the title misc/wasm: huge copy overhead when passing byte slices over Go/JS boundary misc/wasm: huge copy overhead when passing byte slices over certain Go/JS boundaries Jul 3, 2018
@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 3, 2018
@leidegre
Copy link
Author

leidegre commented Jul 3, 2018

@neelance this might interest you as well.

@neelance
Copy link
Member

neelance commented Jul 3, 2018

The buffer of the typed array is the whole WebAssembly memory. You take a slice of that buffer and then pass it to postMessage. I'm not sure how postMessage is supposed to work on buffers, but it is most likely the culprit. I guess it serializes the whole buffer, not only the slice. You should try to actually create a second buffer with only the data you need.

@agnivade agnivade changed the title misc/wasm: huge copy overhead when passing byte slices over certain Go/JS boundaries syscall/js: huge copy overhead when passing byte slices over certain Go/JS boundaries Jul 3, 2018
@agnivade agnivade added the arch-wasm WebAssembly issues label Jul 3, 2018
@agnivade agnivade added this to the Go1.12 milestone Jul 3, 2018
@agnivade
Copy link
Contributor

agnivade commented Jul 3, 2018

@neelance - Feel free to comment if you want to prioritize it for 1.11.

@neelance
Copy link
Member

neelance commented Jul 3, 2018

If this is really due to postMessage, then it is working as intended from Go's point of view. postMessage shouldn't be used that way. @leidegre needs to evaluate if this is really the case, then we can close.

@agnivade agnivade added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 3, 2018
@leidegre
Copy link
Author

leidegre commented Jul 4, 2018

I should probably have emphasized this a bit more but the issue is more subtle than postMessage. For example, if you do console.log on a slice, you run into the same issue and I suspect that there are additional edge cases lurking around the corner. If you trigger this behavior your whole app goes up in the air.

For example, take this program as an example

package main

func main() {
	var x []byte
	println(x)
}

You run that 3 or 4 times and the process hosting the page hard faults with an out of memory error and it is not limited to []byte an println([]int{}) is just as dangerous and please note that this behavior is not limited to WebWorker processes.

@neelance what do you think people will be doing with this, as soon as it's in the offical Go release? I think, at the very least, that this warrants a disclaimer. People will probably run into inexplicable crashes and having some guidance on this behavior accompanying the release, could be a good idea. Save some headaches.

I should also emphasize again that this workaround, where the slice is first explicitly copied into a memory segment managed by the JavaScript GC works really well. It doesn't have the same problem when passed to for example console.log or postMessage

[worker.js]

// I changed the name from slice to copy, becuase it is copying a portion of the ArrayBuffer
function copy({ byteOffset, byteLength, buffer }) {
  return buffer.slice(byteOffset, byteOffset + byteLength)
}

[worker.go]

var x []byte
x = append(x, 2)
x = append(x, 3)
x = append(x, 5)
y := js.TypedArrayOf(x)
z := global.Call("copy", y)
y.Release()

global.Call("postMessage", z) // from WebWorker postMessage

Additionally, the reason why I'm doing this, like this is that I'm having my Go program do expensive crypto stuff, stuff that isn't supported by any other major npm crypto package that I know of. Being able to leverage the Go standard library has tremendous value in this case. Also, there doesn't seem to be an obvious way to build a dynamic object in Go and have that pass the boundary, i.e. postMessage so I opted to pass already serialized JSON and use JSON.Parse on the browser receiving side. Since the json.Marshal Go function return []byte, error it was just natural to do it this way. If there is a standard way of serializing data to JSON in the library, I suspect, people will use it but you end up with []byte not string, if you convert this value to string before you send it, you avoid the issue entirely but if you don't you end up running out of memory very quickly.

@neelance
Copy link
Member

neelance commented Jul 4, 2018

You run that 3 or 4 times and the process hosting the page hard faults with an out of memory error and it is not limited to []byte an println([]int{}) is just as dangerous and please note that this behavior is not limited to WebWorker processes.

I can not reproduce this on Chrome, Firefox or Safari.

@leidegre
Copy link
Author

leidegre commented Jul 4, 2018

@neelance that's new. What build where you using? I can test on additional systems. Also I will put up my repo somewhere for you to try.

@leidegre
Copy link
Author

leidegre commented Jul 4, 2018

@neelance is this Windows specific?

Exact code I used

https://gist.github.com/leidegre/2fe06162eae09837ab533d6acf5d90e6

Git clone/download as zip then npx serve . goto http://localhost:5000/wasm_exec.html

Click Run button a couple of times and I get "Aw, Snap".

Chrome Version 67.0.3396.99 (Official Build) (64-bit) (cohort: Stable)

@leidegre
Copy link
Author

leidegre commented Jul 4, 2018

@neelance I promise you, I'm not making this up

recording

@leidegre
Copy link
Author

leidegre commented Jul 4, 2018

@neelance watch the "Committed" value after I open the devtools. I managed to crash another processes on the system as well because I ran out of virtual memory.

recording

Firefox 61.0 (64-bit)

@yml
Copy link

yml commented Jul 5, 2018

I can observe similar behavior on FF linux. There is a linear growth as you click on the Run button

image
The 2 snapshots above have been taken respectively after the first click and a after few hundred clicks.

@leidegre
Copy link
Author

leidegre commented Jul 5, 2018

I did a quick test with Debian (4.9.82-1+deb9u3) and Chromium 66

As I click run, the virtual memory expands to the point I hit my limit, which is 16 GiB, at that point I get an out of memory error and subsequent attempts to run the program fail with the error message shown in the log.

screenshot

I'm not trying to point out a flaw with the JS/WASM approach, simply that this is an issue that people will run into and it will cause all kinds of weird faults, which running out of virtual memory does. I think we need to be mindful of when values mapped in memory in Go, traverse a boundary like this. A simple solution is to introduce copying so that the ArrayBuffer matches perfectly the value where needed would work. I think this is something the JS/WASM backend can do.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/122296 mentions this issue: misc/wasm: free up memory on exit

@neelance
Copy link
Member

neelance commented Jul 5, 2018

Unfortunately I still can't reproduce this on my OS X machine, Chrome 67.0.3396.99.

I just used Chrome's memory profiler to look into how many WebAssembly instances are allocated at any given time. The CL above makes it so this number does not go above 1 on my machine. Would you mind looking into the memory profiler on your machine to see if it shows something different?

I believe that It happens on your machine, but I am still not convinced that this is related to the JS/Go boundary. Have you tried a fully empty program, without any println instruction?

@leidegre
Copy link
Author

leidegre commented Jul 5, 2018

@neelance I have not tried a fully empty program with any println code. My belief is that that wouldn't cause an issue. Overall, I'm not having any problems with the JS/WASM tooling, things work really well but I'm purposely avoiding the things that cause the linear memory map to be copied.

This is 100% workable, it's just that if you happen to touch these builtin functions or pass any kind of slice over a JS boundary where copying happens you get this huge memory hit which will cause all kinds of crazy. All this is, is a nasty edge case that the user may run into.

To the extent that we can prevent the user from running into this issue, I think it would be prudent to do so. Unfortunately, I do not know how you could possibly detect that the slice you are about to pass is going to be copied in the most unfortunate manner possible.

I guess you could deal with this in the js.Value type, somehow, but that would imply that this copying should take place all the time you cross a JS/Go boundrary, regardless of where it is actually needed. Until a better solution is available, I'm perfectly fine with leaving this issue as-is or closing as "working as intended" but with some action to include this in a known issue release note, or maybe a blog post. It's an edge case, a nasty one that I just know is gonna put people off and the kind of thing that I've not had to deal with when working with Go in the past. It's very Go unlike, even if it's JavaScript and the whole browser ecosystem's fault.

@leidegre
Copy link
Author

leidegre commented Jul 5, 2018

@neelance maybe we should bring this issue to the browser people?

@rusco
Copy link

rusco commented Jul 6, 2018

On Win10/64 bit with Edge everything works fine, no crash after a lot of clicks.

Here my Browser version:
Microsoft Edge 41.16299.492.0
Microsoft EdgeHTML 16.16299
with enabled about:flags "Enable experimental Javascript features"

For Chrome 67 I can reproduce the crash (after 4 clicks).

gopherbot pushed a commit that referenced this issue Jul 11, 2018
Private fields of the Go class are not used any more after the program
has exited. Delete them to allow JavaScript's garbage collection to
clean up the WebAssembly instance.

Updates #26193.

Change-Id: I349784a49eaad0c22ceedd4f859df97132775537
Reviewed-on: https://go-review.googlesource.com/122296
Run-TryBot: Richard Musiol <neelance@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Paul Jolly <paul@myitcv.org.uk>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@agnivade agnivade removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Aug 10, 2018
@neelance
Copy link
Member

@leidegre Are you still able to reproduce this with the latest Chromium?

@leidegre
Copy link
Author

@neelance not with latest build of Chromium no, but I can reliably crash Chrome 69 and 70.

So, it is a browser thing?

  • Version 72.0.3585.0 (Developer Build) (64-bit) works.
  • Version 70.0.3538.67 (Official Build) (64-bit) does not work

@neelance
Copy link
Member

Yes, it is. Is it okay to close this?

@leidegre
Copy link
Author

Sure, but until this is fixed in all browser people will be crashing hard. Hopefully they'll find this.

@golang golang locked and limited conversation to collaborators Oct 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants