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.

Sign up for GitHub

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

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

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

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

twifkak opened this issue Dec 20, 2018 · 10 comments
Labels
arch-wasm WebAssembly issues Documentation Issues describing a change to documentation. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unplanned

Comments

@twifkak
Copy link

twifkak commented Dec 20, 2018 •
edited
Loading

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

twifkak commented Dec 20, 2018

@neelance

@neelance neelance added the arch-wasm WebAssembly issues label Dec 20, 2018
@neelance
Copy link
Member

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 20, 2018
@agnivade agnivade added this to the Unplanned milestone Dec 20, 2018
@twifkak
Copy link
Author

twifkak commented Dec 20, 2018 •
edited
Loading

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
@twifkak
Reduce initial Go heap from 768M to 128M.
51ebd75 
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 twifkak mentioned this issue Dec 20, 2018
Merged
twifkak added a commit to ampproject/amppackager that referenced this issue Dec 20, 2018
@twifkak
Reduce initial Go heap from 768M to 128M. (#221)
170f65a 
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
Copy link
Member

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

twifkak commented Dec 22, 2018

OK, fair enough. Thanks!

@agnivade
Copy link
Contributor

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

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

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

@neelance
Copy link
Member

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 gopherbot added the Documentation Issues describing a change to documentation. label Dec 26, 2018
@agnivade agnivade added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Dec 26, 2018
@gopherbot
Copy link
Contributor

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

@golang golang locked and limited conversation to collaborators Feb 27, 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 Documentation Issues describing a change to documentation. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
4 participants
@twifkak @neelance @agnivade @gopherbot