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

fs: extracts syscallfs.ReaderAtOffset for WASI and gojs #1037

Merged
merged 2 commits into from
Jan 16, 2023

Conversation

codefromthecrypt
Copy link
Contributor

This extracts a utility syscallfs.ReaderAtOffset() to allow WASI and gojs to re-use the same logic to implement syscall.Pread.

What's different than before is that if WASI passes multiple iovecs an emulated ReaderAt will seek to the read position on each call to Read vs once per loop. This was a design decision to keep the call sites compatible between files that implement ReaderAt and those that emulate them with Seeker (e.g. avoid the need for a read-scoped closer/ defer function). The main use case for emulation is embed.file, whose seek function is cheap, so there's little performance impact to this.

goos: darwin
goarch: amd64
pkg: github.com/tetratelabs/wazero/internal/syscallfs
cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
BenchmarkReaderAtOffset
BenchmarkReaderAtOffset/os.DirFS_io.File
BenchmarkReaderAtOffset/os.DirFS_io.File-16         	  549439	      2196 ns/op
BenchmarkReaderAtOffset/os.DirFS_readerAtOffset
BenchmarkReaderAtOffset/os.DirFS_readerAtOffset-16  	  612918	      1985 ns/op
BenchmarkReaderAtOffset/embed.FS_embed.file
BenchmarkReaderAtOffset/embed.FS_embed.file-16      	10059037	       126.1 ns/op
BenchmarkReaderAtOffset/embed.FS_seekToOffsetReader
BenchmarkReaderAtOffset/embed.FS_seekToOffsetReader-16         	 7055109	       179.0 ns/op

@codefromthecrypt
Copy link
Contributor Author

cc @ncruces as this moves around the stuff you made, and thanks for that!

@codefromthecrypt codefromthecrypt mentioned this pull request Jan 15, 2023
5 tasks
@ncruces
Copy link
Contributor

ncruces commented Jan 15, 2023

I'll try to have a stab at convincing the Go folks to add ReaderAt to embed. There's no good reason to for it not to. But that's always at least a year off.

@codefromthecrypt
Copy link
Contributor Author

I'll try to have a stab at convincing the Go folks to add ReaderAt to embed. There's no good reason to for it not too. But that's always at least a year off.

Thanks, I scratched my head looking at it, too, as it seems as easy as mapfs. You are right though, there is a socialization, review and landing concern. Thanks for offering to take that on which is much more effort than the code and tests!

This extracts a utility `syscallfs.ReaderAtOffset()` to allow WASI and
gojs to re-use the same logic to implement `syscall.Pread`.

What's different than before is that if WASI passes multiple iovecs an
emulated `ReaderAt` will seek to the read position on each call to
`Read` vs once per loop. This was a design decision to keep the call
sites compatible between files that implement ReaderAt and those that
emulate them with Seeker (e.g. avoid the need for a read-scoped closer/
defer function). The main use case for emulation is `embed.file`, whose
seek function is cheap, so there's little performance impact to this.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt codefromthecrypt merged commit 713e187 into main Jan 16, 2023
@codefromthecrypt codefromthecrypt deleted the ReaderAtOffset branch January 16, 2023 01:30
@ncruces
Copy link
Contributor

ncruces commented Apr 12, 2023

Got it merged, should be in 1.21:
golang/go#57803 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants