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

cmd/compile: document that TypedArrays are not pinned in memory #29355

Closed
twifkak opened this issue Dec 20, 2018 · 10 comments

Comments

Projects
None yet
4 participants
@twifkak
Copy link

commented Dec 20, 2018

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

$ go version
go version go1.11.4 linux/amd64

What version of Node are you using?

$ node --version
v10.13.0

Does this issue reproduce with the latest release?

Yes. This occurs at master, as well.

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/twifkak/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="//home/twifkak/.go"
GOPROXY=""
GORACE=""
GOROOT="/home/twifkak/usr/opt/go"
GOTMPDIR=""
GOTOOLDIR="/home/twifkak/usr/opt/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build919054145=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Created a js.TypedArray in a Go program running under wasm, and then mutated it in a JS callback.

What did you expect to see?

The TypedArray would be successfully mutated, which the Go program could subsequently inspect.

What did you see instead?

In some circumstances, I would get:

TypeError: Cannot perform %TypedArray%.prototype.set on a detached ArrayBuffer

My suspicion is that the GC is moving the underlying byte arrays in times of memory pressure, because the easiest way to trigger this is to decrease the initial heap size, either using wams or by modifying asm.go.

Background

I'm doing this to keep a long-running Go-wasm process around and repeatedly call a Go function from JS, without leaking the input/output objects due to lack of cross-heap GC.

The code to reproduce it is at ampproject/amppackager#220 but requires a test file I haven't checked in -- if you're interested in running it, let me know and I'll add one.

Let me know if I'm holding it wrong and there's some simpler way of achieving my goal.

@twifkak

This comment has been minimized.

Copy link
Author

commented Dec 20, 2018

@neelance neelance added the Arch-Wasm label Dec 20, 2018

@neelance

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

Hmm, this is unfortunate. Calling memory.grow (which may happen at any time) invalidates the existing ArrayBuffer for the memory. Since typed arrays are based on this ArrayBuffer, they stop working as well.

With the current state of WebAssembly the only solution that I see is to not pass the typed array directly but instead wrap it in a custom helper that can create the correct typed array on demand and the user is not allowed to keep a reference to this typed array. This is quite cumbersome.

The threads proposal might save us: It used a SharedArrayBuffer instead of an ArrayBuffer and it seems like it would never detach.

@agnivade agnivade changed the title wasm: TypedArrays are not pinned in memory cmd/compile: TypedArrays are not pinned in memory Dec 20, 2018

@agnivade agnivade added this to the Unplanned milestone Dec 20, 2018

@twifkak

This comment has been minimized.

Copy link
Author

commented Dec 20, 2018

Ah, I see, thanks for the explanation! I was able to get the wrapper approach to work, but boy is it a lot of cruft. Also, will this leak the TypedArray pointers over time?

The threads proposal is indeed interesting. Until the proposal lands, would it be possible to create a second, non-growing memory instance to be used as an arena to communicate over?

twifkak added a commit to twifkak/amppackager that referenced this issue Dec 20, 2018

Reduce initial Go heap from 768M to 128M.
The 128M number was arbitrary, and some tuning may be necessary to find
an optimal number.

This ends up triggering golang/go#29355, and thus requires the
workaround suggested in that thread: construct a flyweight TypedArray
each type the JS wants to mutate the slice.

The workaround slows things down around 18% from ~39ms per document to
~46ms, but I have an idea for a workaround for _that_ (TODO in the
code).

Delete the Go1.12 port because it's not worth maintaining it at this
early prototypal stage.

twifkak added a commit to ampproject/amppackager that referenced this issue Dec 20, 2018

Reduce initial Go heap from 768M to 128M. (#221)
The 128M number was arbitrary, and some tuning may be necessary to find
an optimal number.

This ends up triggering golang/go#29355, and thus requires the
workaround suggested in that thread: construct a flyweight TypedArray
each type the JS wants to mutate the slice.

The workaround slows things down around 18% from ~39ms per document to
~46ms, but I have an idea for a workaround for _that_ (TODO in the
code).

Delete the Go1.12 port because it's not worth maintaining it at this
early prototypal stage.
@neelance

This comment has been minimized.

Copy link
Member

commented Dec 21, 2018

I was able to get the wrapper approach to work, but boy is it a lot of cruft. Also, will this leak the TypedArray pointers over time?

Depends on your implementation. I'm not sure what you did.

The threads proposal is indeed interesting. Until the proposal lands, would it be possible to create a second, non-growing memory instance to be used as an arena to communicate over?

I don't see a good way to make this work with Go's memory management.

@twifkak

This comment has been minimized.

Copy link
Author

commented Dec 22, 2018

OK, fair enough. Thanks!

@agnivade

This comment has been minimized.

Copy link
Member

commented Dec 22, 2018

@neelance - Do you want to keep this bug open until thread support is implemented (which is already being tracked here #28631) ? Or is there something that can be done prior ?

@neelance

This comment has been minimized.

Copy link
Member

commented Dec 22, 2018

I currently don't see anything worthwhile that we could do prior. You can still use typed arrays right now, you just shouldn't hold a reference to them from JavaScript. For example the way that the net/http code is using them is fine.

@agnivade

This comment has been minimized.

Copy link
Member

commented Dec 23, 2018

Does it make sense to add a line of documentation clarifying this ?

@neelance

This comment has been minimized.

Copy link
Member

commented Dec 25, 2018

Sure, why not.

@agnivade agnivade changed the title cmd/compile: TypedArrays are not pinned in memory cmd/compile: document that TypedArrays are not pinned in memory Dec 26, 2018

@gopherbot

This comment has been minimized.

Copy link

commented Dec 26, 2018

Change https://golang.org/cl/155778 mentions this issue: syscall/js: clarify that TypedArray should not be modified from JS

@gopherbot gopherbot closed this in 4373281 Feb 27, 2019

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.