-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
syscall/js, io/fs: fs.Read copies minimally 512 bytes to from JS to Go #53566
Comments
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 cc @bradfitz as I think you've seen some code around this in the past (hint |
@ianlancetaylor can you give this a look? |
This is js so @neelance |
@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. 👍 |
This reduces impact of EOF or other short reads when `GOOS=js`. Fixes golang#53566 Signed-off-by: Adrian Cole <adrian@tetrate.io>
This reduces impact of EOF or other short reads when `GOOS=js`. Fixes golang#53566
ok got around to it #55062 |
Change https://go.dev/cl/430875 mentions this issue: |
This reduces impact of EOF or other short reads when `GOOS=js`. Fixes golang#53566 Signed-off-by: Adrian Cole <adrian@tetrate.io>
This reduces impact of EOF or other short reads when `GOOS=js`. Fixes golang#53566 Signed-off-by: Adrian Cole <adrian@tetrate.io>
This reduces impact of EOF or other short reads when `GOOS=js`. Fixes golang#53566
What version of Go are you using (
go version
)?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
OutputWhat did you do?
Compiled a program that ends up using fs.Read and fs.Write.
Ex.
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 andthe 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
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.
The text was updated successfully, but these errors were encountered: