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

Jump to bottom

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

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

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

eliasnaur opened this issue May 11, 2019 · 6 comments
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.13

Comments

@eliasnaur
Copy link
Contributor

eliasnaur commented May 11, 2019 •
edited

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 13, 2019
@andybons andybons added this to the Unplanned milestone May 13, 2019
@andybons andybons added the arch-wasm WebAssembly issues label May 13, 2019
@bradfitz bradfitz modified the milestones: Unplanned, Go1.13 May 14, 2019
@bradfitz
Copy link
Contributor

@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
Copy link
Member

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
Copy link

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

@neelance
Copy link
Member

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

@eliasnaur
Copy link
Contributor Author

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
Copy link
Member

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.

@golang golang locked and limited conversation to collaborators May 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
6 participants
@bradfitz @neelance @andybons @eliasnaur @gopherbot @cherrymui