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

misc/wasm: js.TypedArrayOf is impossible to use correctly #31980

Open
eliasnaur opened this issue May 11, 2019 · 6 comments

Comments

Projects
None yet
6 participants
@eliasnaur
Copy link
Contributor

commented May 11, 2019

I'm working on a WebAssembly port of Gio, using WebGL for drawing. Some WebGL functions such as bufferData take a TypedArray for efficient data transfer. Calling bufferData from Go looks like this:

var ctx js.Value = ... // WebGLRenderingContext
var data []byte = ...

array := js.TypedArrayOf(data)
ctx.Call("bufferData", ..., array, ...)
array.Release()

However, quoting from syscall/js:

The typed array currently becomes inaccessible when Go requests more memory from the WebAssembly host. It is recommended to only use the typed array synchronously without keeping a long-lived reference. You can also check if the length property is zero to detect this detached state of the typed array.

CL 174304 works around one instance of this problem and replaced the typed array with a copy. Converting our bufferData call above in a similar way gives:

array := js.Global().Get("Uint8Array").New(len(data))
dataArr := js.TypedArrayOf(data)
array.Call("set", dataArr)
dataArr.Release()
ctx.Call("bufferData", ..., array, ...)

However, the above still results in rare crashes, leading me to suspect that there is no way to use TypedArrayOf correctly. I failed to produce a minimal test program, but I believe a modified version of Gio is enough to demonstrate the issue.

$ node --version
v12.1.0
$ go version
go version devel +4cd6c3bac7 Wed May 8 16:00:05 2019 +0000 darwin/amd64
$ git clone git@git.sr.ht:~eliasnaur/webasm-crash-demo
$ cd webasm-crash-demo/apps/
$ GOOS=js GOARCH=wasm go run -exec $GOROOT/misc/wasm/go_js_wasm_exec ./gophers/
panic: JavaScript error: Cannot perform %TypedArray%.prototype.set on a neutered ArrayBuffer

goroutine 1 [running]:
syscall/js.Value.Call(0x7ff8000000000015, 0x157750, 0x3, 0xc5aec8, 0x1, 0x1, 0x12b3000d)
	/Users/elias/go-tip/src/syscall/js/js.go:325 +0x82
gioui.org/ui/app/internal/gl.byteArrayOf(0xc2a140, 0x40, 0x40, 0xc2a140)
	/Users/elias/scratch/webasm-crash/webasm-crash-demo/ui/app/internal/gl/gl_js.go:325 +0xa
gioui.org/ui/app/internal/gl.init.0()
	/Users/elias/scratch/webasm-crash/webasm-crash-demo/ui/app/internal/gl/gl_js.go:304 +0x3
exit status 2

The relevant code is

func init() {
    byteArrayOf(make([]byte, 64))
}
func byteArrayOf(data []byte) js.Value {
    if len(data) == 0 {
        return js.Null()
    }
    var a js.Value
    a = uint8Array.New(len(data))
    s := js.TypedArrayOf(data)
    runtime.GC()
    a.Call("set", s)
    s.Release()
    return a
}

The runtime.GC call is required to reproduce the crash every run, but in my case the crash happens even in the browser and without the runtime.GC call.

If I'm right, CL 174304 merely narrows the window for a crash, and #31812 is difficult to fix.

In order of preference, I propose one of:

  • Fixing TypedArrayOf to not have this problem. Reading the syscall/js note, how can I make "synchronous" calls to WebGL? Or just synchronous "set" calls to a typedarray backed by javascript memory?
  • Expand syscall/js.Value.Call (and Invoke) to accept Go slices, which are atomically wrapped in a TypedArray before being passed to the native side.
  • Add syscall/js.CopySlice (or similar) that atomically copies a Go slice into a (JS backed or not) typed array.

CC @neelance.

@andybons andybons added this to the Unplanned milestone May 13, 2019

@andybons andybons added the Arch-Wasm label May 13, 2019

@bradfitz bradfitz modified the milestones: Unplanned, Go1.13 May 14, 2019

@bradfitz

This comment has been minimized.

Copy link
Member

commented May 14, 2019

@neelance, @cherrymui, can we figure out a plan here for Go 1.13? Between stuff like this and #31812, it does seem like some API changes are needed.

@neelance

This comment has been minimized.

Copy link
Member

commented May 14, 2019

Yes, I agree that this is currently a bad situation.

It is still unclear to me how WebAssembly will address this situation in the long run. With threads, the memory can grow at any time, even in parallel to the execution of JavaScript code. There is some discussion on WebAssembly/design#1271 but no consensus yet.

The most safe approach right now would probably be to remove TypedArrayOf and provide functions that atomically copy between a byte slice and an ArrayBuffer.

@gopherbot

This comment has been minimized.

Copy link

commented May 15, 2019

Change https://golang.org/cl/177537 mentions this issue: syscall/js: replace TypedArrayOf with ReadBytes/WriteBytes

@neelance

This comment has been minimized.

Copy link
Member

commented May 15, 2019

I have drafted the proposed solution here: https://golang.org/cl/177537
Are there any good alternatives?

@eliasnaur

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

Have you considering my second proposal above? Namely: expand syscall/js.Value.Call (and Invoke) to accept Go slices, which are atomically wrapped in a TypedArray before being passed to the native side. I believe that's a strictly better option than explicit copy to/from JS:

  • No new API. CopyFromJS and CopyToJS can be done in user code:
var s []byte
ta := js.Global().Get("document").GetGet("Uint8Array").New(len(s))
ta.Call("set", s)

where the Call atomically transfers the slice s to JS.

  • No data copying. This is important for large arrays and performance (WebGL in particular). If atomic wrapping is impossible today, we could relax the semantics and allow the runtime to (atomically) copy the Go slice to a JS typed array before the call.
  • No extra JS calls to Copy are required for a JS call with typed array arguments. JS calls are expensive in Go wasm.
@neelance

This comment has been minimized.

Copy link
Member

commented May 16, 2019

The problem with the second approach is that it will still cause issues if the function that you are calling is using the typed array asynchronously. For example it would not have prevented #31702. I currently believe that there are quite some APIs like this. Your example with bufferData seems to be fully synchronous, so it might work, but right now I would prefer to switch to a solution that can't easily be used in a wrong way. Maybe at some point WebAssembly will use a SharedArrayBuffer which behaves better, then we can consider to restore TypedArrayOf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.