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

syscall/js, io/fs: fs.Read copies minimally 512 bytes to from JS to Go #53566

Open
codefromthecrypt opened this issue Jun 27, 2022 · 6 comments
Open

syscall/js, io/fs: fs.Read copies minimally 512 bytes to from JS to Go #53566

codefromthecrypt opened this issue Jun 27, 2022 · 6 comments
Labels
arch-wasm WebAssembly issues NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unplanned

Comments

@codefromthecrypt
Copy link

codefromthecrypt commented Jun 27, 2022 •
edited

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

$ go version
go version go1.17.11 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

GOARCH=wasm GOOS=js

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/adrian/Library/Caches/go-build"
GOENV="/Users/adrian/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/adrian/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/adrian/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/adrian/.gimme/versions/go1.17.11.darwin.amd64"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/adrian/.gimme/versions/go1.17.11.darwin.amd64/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.17.11"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/adrian/oss/wazero/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/vd/1cf8zdb1721f4z5rjggy8bp40000gn/T/go-build3326543425=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Compiled a program that ends up using fs.Read and fs.Write.

Ex.

bytes, err := ioutil.ReadFile(os.Args[i])
if err != nil {
	os.Exit(1)
}
os.Stdout.Write(bytes)

What did you expect to see?

I expected the resulting wasm to js.copyBytesToGo no more than the size of the file.

What did you see instead?

js.copyBytesToGo was called with 512 instead because the contents were small and
the nRead is currently ignored in fs_js.go. It is 512 due to special casing upstream for
small files in os.ReadFile.

Here's the snippet of fs.Read

	buf := uint8Array.New(len(b))
	n, err := fsCall("read", fd, buf, 0, len(b), nil)
	if err != nil {
		return 0, err
	}
	js.CopyBytesToGo(b, buf) // < not buf[0:n]

I think this should only copy n bytes as it is more efficient.
It should also skip invoking js.CopyBytesToGo when zero bytes were read.
@codefromthecrypt
Copy link
Author

I'm pretty sure this small change will make a lot of impact and happy to implement it unless it is easier for folks already here to do it.

Note this is particularly troublesome as fsCall cannot return EOF due to there being no way to construct it in mapJSError. What this means is that a 17 byte file ends up with two buffer copies.. one 512 to copy the actual 17 bytes and another 495 byte copy (512-17) for the zero byte read mapped heuristically to to EOF due to the lack of EOF support in GOOS=js

cc @bradfitz as I think you've seen some code around this in the past (hint poll.FD.Read)

@dmitshur dmitshur changed the title js/fs: fs.Read copies minimally 512 bytes to from JS to Go syscall/js, io/fs: fs.Read copies minimally 512 bytes to from JS to Go Jun 27, 2022
@dr2chase
Copy link
Contributor

dr2chase commented Jul 1, 2022

@ianlancetaylor can you give this a look?

@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 1, 2022
@ianlancetaylor
Copy link
Contributor

This is js so @neelance

@neelance
Copy link
Member

@codefromthecrypt I have just reproduced your findings. Feel free to implement a fix. I'd be very happy to see you help with Go's support for WebAssembly. 👍

@agnivade agnivade added NeedsFix The path to resolution is known, but the work has not been done. arch-wasm WebAssembly issues and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 25, 2022
@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
codefromthecrypt added a commit to codefromthecrypt/go that referenced this issue Sep 14, 2022
@codefromthecrypt
syscall/js, io/fs: Makes copies from JS to Go more efficient
74ccc7a 
This reduces impact of EOF or other short reads when `GOOS=js`.

Fixes golang#53566

Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt added a commit to codefromthecrypt/go that referenced this issue Sep 14, 2022
@codefromthecrypt
syscall/js: Makes copies from JS to Go more efficient
2e5b7b2 
This reduces impact of EOF or other short reads when `GOOS=js`.

Fixes golang#53566
@codefromthecrypt codefromthecrypt mentioned this issue Sep 14, 2022
Closed
@codefromthecrypt
Copy link
Author

ok got around to it #55062

@gopherbot
Copy link

Change https://go.dev/cl/430875 mentions this issue: syscall/js: Makes copies from JS to Go more efficient

codefromthecrypt added a commit to codefromthecrypt/go that referenced this issue Oct 23, 2022
@codefromthecrypt
syscall/js: Makes copies from JS to Go more efficient
7941411 
This reduces impact of EOF or other short reads when `GOOS=js`.

Fixes golang#53566

Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt added a commit to codefromthecrypt/go that referenced this issue Jan 2, 2023
@codefromthecrypt
syscall: Makes copies from JS to Go more efficient
d9390db 
This reduces impact of EOF or other short reads when `GOOS=js`.

Fixes golang#53566

Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt added a commit to codefromthecrypt/go that referenced this issue Feb 1, 2023
@codefromthecrypt
syscall: return early from CopyBytesToGo when dst is empty
50dbd82 
This reduces impact of EOF or other short reads when `GOOS=js`.

Fixes golang#53566
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
arch-wasm WebAssembly issues NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unplanned
Development

Successfully merging a pull request may close this issue.

syscall: Makes copies from JS to Go more efficient codefromthecrypt/go
7 participants
@neelance @codefromthecrypt @agnivade @dr2chase @ianlancetaylor @gopherbot @seankhliao