Repository: tetratelabs/wazero
Pull request #914 (2022-12-12 07:06:38)

wasi: skips fd allocation on path_filestat_get and backfills benchmarks
```
goos: darwin
goarch: amd64
pkg: github.com/tetratelabs/wazero/imports/wasi_snapshot_preview1
cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
Benchmark_pathFilestat
Benchmark_pathFilestat/embed.FS_fd=root
Benchmark_pathFilestat/embed.FS_fd=root-16         	  410125	      2712 ns/op	     144 B/op	       6 allocs/op
Benchmark_pathFilestat/embed.FS_fd=directory
Benchmark_pathFilestat/embed.FS_fd=directory-16    	  461304	      2717 ns/op	     160 B/op	       8 allocs/op
Benchmark_pathFilestat/os.DirFS_fd=root
Benchmark_pathFilestat/os.DirFS_fd=root-16         	   58639	     21299 ns/op	     408 B/op	       8 allocs/op
Benchmark_pathFilestat/os.DirFS_fd=directory
Benchmark_pathFilestat/os.DirFS_fd=directory-16    	   56630	     21274 ns/op	     456 B/op
```


Patch:
diff --git a/imports/wasi_snapshot_preview1/fs.go b/imports/wasi_snapshot_preview1/fs.go
index 79a31180..554e5ad9 100644
--- a/imports/wasi_snapshot_preview1/fs.go
+++ b/imports/wasi_snapshot_preview1/fs.go
@@ -310,6 +310,18 @@ func fdFilestatGetFn(ctx context.Context, mod api.Module, params []uint64) Errno
 	return fdFilestatGetFunc(ctx, mod, uint32(params[0]), uint32(params[1]))
 }
 
+// TODO: support filesize and mtime when root
+var rootFilestat = []byte{
+	0, 0, 0, 0, 0, 0, 0, 0, // device
+	0, 0, 0, 0, 0, 0, 0, 0, // inode
+	byte(wasiFiletypeDirectory), 0, 0, 0, 0, 0, 0, 0, // filetype
+	1, 0, 0, 0, 0, 0, 0, 0, // nlink
+	0, 0, 0, 0, 0, 0, 0, 0, // filesize
+	0, 0, 0, 0, 0, 0, 0, 0, // atim
+	0, 0, 0, 0, 0, 0, 0, 0, // mtim
+	0, 0, 0, 0, 0, 0, 0, 0, // ctim
+}
+
 func fdFilestatGetFunc(ctx context.Context, mod api.Module, fd, resultBuf uint32) Errno {
 	// Ensure we can write the filestat
 	buf, ok := mod.Memory().Read(ctx, resultBuf, 64)
@@ -329,25 +341,17 @@ func fdFilestatGetFunc(ctx context.Context, mod api.Module, fd, resultBuf uint32
 	file, ok := sysCtx.FS(ctx).OpenedFile(ctx, fd)
 	if !ok {
 		return ErrnoBadf
+	} else if fd == internalsys.FdRoot {
+		copy(buf, rootFilestat)
+		return ErrnoSuccess
 	}
 
-	var filetype wasiFiletype
-	var filesize uint64
-	var mtim int64
-	if f := file.File; f != nil { // not root
-		if stat, err := f.Stat(); err != nil {
-			return ErrnoIo
-		} else {
-			filetype = getWasiFiletype(stat.Mode())
-			filesize = uint64(stat.Size())
-			mtim = stat.ModTime().UnixNano()
-		}
-	} else { // root
-		filetype = wasiFiletypeDirectory
-		// TODO: support filesize and mtime when root
+	stat, err := file.File.Stat()
+	if err != nil {
+		return ErrnoIo
 	}
 
-	writeFilestat(buf, filetype, filesize, mtim)
+	writeFilestat(buf, stat)
 
 	return ErrnoSuccess
 }
@@ -379,7 +383,11 @@ var charFilestat = []byte{
 	0, 0, 0, 0, 0, 0, 0, 0, // ctim
 }
 
-func writeFilestat(buf []byte, filetype wasiFiletype, filesize uint64, mtim int64) {
+func writeFilestat(buf []byte, stat fs.FileInfo) {
+	filetype := getWasiFiletype(stat.Mode())
+	filesize := uint64(stat.Size())
+	mtim := stat.ModTime().UnixNano()
+
 	// memory is re-used, so ensure the result is defaulted.
 	copy(buf, charFilestat[:32])
 	buf[16] = uint8(filetype)
@@ -1203,11 +1211,13 @@ func pathFilestatGetFn(ctx context.Context, mod api.Module, params []uint64) Err
 
 	fd := uint32(params[0])
 
-	// TODO: implement flags?
-	// flags := uint32(params[1])
+	// TODO: flags is a lookupflags and it only has one bit: symlink_follow
+	// https://github.com/WebAssembly/WASI/blob/snapshot-01/phases/snapshot/docs.md#lookupflags
+	_ /* flags */ = uint32(params[1])
 
 	pathOffset := uint32(params[2])
 	pathLen := uint32(params[3])
+
 	resultBuf := uint32(params[4])
 
 	// open_at isn't supported in fs.FS, so we check the path can't escape,
@@ -1218,6 +1228,7 @@ func pathFilestatGetFn(ctx context.Context, mod api.Module, params []uint64) Err
 	}
 	pathName := string(b)
 
+	// Prepend the path if necessary.
 	if dir, ok := fsc.OpenedFile(ctx, fd); !ok {
 		return ErrnoBadf
 	} else if dir.File == nil { // root
@@ -1227,15 +1238,20 @@ func pathFilestatGetFn(ctx context.Context, mod api.Module, params []uint64) Err
 		pathName = path.Join(dir.Path, pathName)
 	}
 
-	// Sadly, we need to open the file to stat it.
-	pathFd, errnoResult := openFile(ctx, fsc, pathName)
+	// Stat the file without allocating a file descriptor
+	stat, errnoResult := statFile(ctx, fsc, pathName)
 	if errnoResult != ErrnoSuccess {
 		return errnoResult
 	}
 
-	// Close it when the function returns.
-	defer fsc.CloseFile(ctx, pathFd)
-	return fdFilestatGetFunc(ctx, mod, pathFd, resultBuf)
+	// Write the stat result to memory
+	buf, ok := mod.Memory().Read(ctx, resultBuf, 64)
+	if !ok {
+		return ErrnoFault
+	}
+	writeFilestat(buf, stat)
+
+	return ErrnoSuccess
 }
 
 // pathFilestatSetTimes is the WASI function named pathFilestatSetTimesName
@@ -1325,13 +1341,21 @@ func pathOpenFn(ctx context.Context, mod api.Module, params []uint64) (uint32, E
 	fsc := sysCtx.FS(ctx)
 
 	fd := uint32(params[0])
+
+	// TODO: dirflags is a lookupflags and it only has one bit: symlink_follow
+	// https://github.com/WebAssembly/WASI/blob/snapshot-01/phases/snapshot/docs.md#lookupflags
 	_ /* dirflags */ = uint32(params[1])
+
 	path := uint32(params[2])
 	pathLen := uint32(params[3])
+
 	_ /* oflags */ = uint32(params[4])
+
 	// rights aren't used
 	_, _ = params[5], params[6]
-	_ /* fdflags */ = uint32(params[7])
+
+	// TODO: only notable fdflag for opening is wasiFdflagsAppend
+	_ /* fdflags */ = wasiFdflags(uint32(params[7]))
 
 	if _, ok := fsc.OpenedFile(ctx, fd); !ok {
 		return 0, ErrnoBadf
@@ -1399,11 +1423,7 @@ var pathUnlinkFile = stubFunction(
 )
 
 // openFile attempts to open the file at the given path. Errors coerce to WASI
-// Errno, returned as a slice to avoid allocation per-error.
-//
-// Note: Coercion isn't centralized in internalsys.FSContext because ABI use
-// different error codes. For example, wasi-filesystem and GOOS=js don't map to
-// these Errno.
+// Errno.
 func openFile(ctx context.Context, fsc *internalsys.FSContext, name string) (fd uint32, errno Errno) {
 	newFD, err := fsc.OpenFile(ctx, name)
 	if err == nil {
@@ -1411,20 +1431,42 @@ func openFile(ctx context.Context, fsc *internalsys.FSContext, name string) (fd
 		errno = ErrnoSuccess
 		return
 	}
+	errno = toErrno(err)
+	return
+}
+
+// statFile attempts to stat the file at the given path. Errors coerce to WASI
+// Errno.
+func statFile(ctx context.Context, fsc *internalsys.FSContext, name string) (stat fs.FileInfo, errno Errno) {
+	s, err := fsc.StatFile(ctx, name)
+	if err == nil {
+		stat = s
+		errno = ErrnoSuccess
+		return
+	}
+	errno = toErrno(err)
+	return
+}
+
+// toErrno coerces the error to a WASI Errno.
+//
+// Note: Coercion isn't centralized in sys.FSContext because ABI use different
+// error codes. For example, wasi-filesystem and GOOS=js don't map to these
+// Errno.
+func toErrno(err error) Errno {
 	// handle all the cases of FS.Open or internal to FSContext.OpenFile
 	switch {
 	case errors.Is(err, fs.ErrInvalid):
-		errno = ErrnoInval
+		return ErrnoInval
 	case errors.Is(err, fs.ErrNotExist):
 		// fs.FS is allowed to return this instead of ErrInvalid on an invalid path
-		errno = ErrnoNoent
+		return ErrnoNoent
 	case errors.Is(err, fs.ErrExist):
-		errno = ErrnoExist
+		return ErrnoExist
 	case errors.Is(err, syscall.EBADF):
 		// fsc.OpenFile currently returns this on out of file descriptors
-		errno = ErrnoBadf
+		return ErrnoBadf
 	default:
-		errno = ErrnoIo
+		return ErrnoIo
 	}
-	return
 }
diff --git a/internal/sys/fs.go b/internal/sys/fs.go
index 4ace070d..ab6fb0a6 100644
--- a/internal/sys/fs.go
+++ b/internal/sys/fs.go
@@ -11,9 +11,22 @@ import (
 )
 
 const (
-	FdStdin = iota
+	FdStdin uint32 = iota
 	FdStdout
 	FdStderr
+
+	// FdRoot is the file descriptor of the root ("/") filesystem.
+	//
+	// # Why file descriptor 3?
+	//
+	// While not specified, the most common WASI implementation, wasi-libc, expects
+	// POSIX style file descriptor allocation, where the lowest available number is
+	// used to open the next file. Since 1 and 2 are taken by stdout and stderr,
+	// `root` is assigned 3.
+	//   - https://github.com/WebAssembly/WASI/issues/122
+	//   - https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_14
+	//   - https://github.com/WebAssembly/wasi-libc/blob/wasi-sdk-16/libc-bottom-half/sources/preopens.c#L215
+	FdRoot
 )
 
 // FSKey is a context.Context Value key. It allows overriding fs.FS for WASI.
@@ -42,7 +55,7 @@ type FileEntry struct {
 	// Path was the argument to FSContext.OpenFile
 	Path string
 
-	// File when nil this is the root "/" (fd=3)
+	// File when nil this is the root "/" (fd=FdRoot)
 	File fs.File
 
 	// ReadDir is present when this File is a fs.ReadDirFile and `ReadDir`
@@ -65,7 +78,8 @@ type FSContext struct {
 	// fs is the root ("/") mount.
 	fs fs.FS
 
-	// openedFiles is a map of file descriptor numbers (>=3) to open files (or directories) and defaults to empty.
+	// openedFiles is a map of file descriptor numbers (>=FdRoot) to open files
+	// (or directories) and defaults to empty.
 	// TODO: This is unguarded, so not goroutine-safe!
 	openedFiles map[uint32]*FileEntry
 
@@ -84,18 +98,8 @@ var emptyFSContext = &FSContext{
 
 // NewFSContext creates a FSContext, using the `root` parameter for any paths
 // beginning at "/". If the input is EmptyFS, there is no root filesystem.
-// Otherwise, `root` is assigned file descriptor 3 and the returned context
-// can open files in that file system.
-//
-// Why file descriptor 3?
-//
-// While not specified, the most common WASI implementation, wasi-libc, expects
-// POSIX style file descriptor allocation, where the lowest available number is
-// used to open the next file. Since 1 and 2 are taken by stdout and stderr,
-// `root` is assigned 3.
-//   - https://github.com/WebAssembly/WASI/issues/122
-//   - https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_14
-//   - https://github.com/WebAssembly/wasi-libc/blob/wasi-sdk-16/libc-bottom-half/sources/preopens.c#L215
+// Otherwise, `root` is assigned file descriptor FdRoot and the returned
+// context can open files in that file system.
 func NewFSContext(root fs.FS) *FSContext {
 	if root == EmptyFS {
 		return emptyFSContext
@@ -103,9 +107,9 @@ func NewFSContext(root fs.FS) *FSContext {
 	return &FSContext{
 		fs: root,
 		openedFiles: map[uint32]*FileEntry{
-			3: {Path: "/"},
+			FdRoot: {Path: "/"},
 		},
-		lastFD: 3,
+		lastFD: FdRoot,
 	}
 }
 
@@ -131,14 +135,7 @@ func (c *FSContext) OpenedFile(_ context.Context, fd uint32) (*FileEntry, bool)
 // e.g. allow os.O_RDONLY, os.O_WRONLY, or os.O_RDWR either by config flag or pattern on filename
 // See #390
 func (c *FSContext) OpenFile(_ context.Context, name string /* TODO: flags int, perm int */) (uint32, error) {
-	// fs.ValidFile cannot be rooted (start with '/')
-	fsOpenPath := name
-	if name[0] == '/' {
-		fsOpenPath = name[1:]
-	}
-	fsOpenPath = path.Clean(fsOpenPath) // e.g. "sub/." -> "sub"
-
-	f, err := c.fs.Open(fsOpenPath)
+	f, err := c.openFile(name)
 	if err != nil {
 		return 0, err
 	}
@@ -152,6 +149,26 @@ func (c *FSContext) OpenFile(_ context.Context, name string /* TODO: flags int,
 	return newFD, nil
 }
 
+func (c *FSContext) StatFile(_ context.Context, name string) (fs.FileInfo, error) {
+	f, err := c.openFile(name)
+	if err != nil {
+		return nil, err
+	}
+	defer f.Close()
+	return f.Stat()
+}
+
+func (c *FSContext) openFile(name string) (fs.File, error) {
+	// fs.ValidFile cannot be rooted (start with '/')
+	fsOpenPath := name
+	if name[0] == '/' {
+		fsOpenPath = name[1:]
+	}
+	fsOpenPath = path.Clean(fsOpenPath) // e.g. "sub/." -> "sub"
+
+	return c.fs.Open(fsOpenPath)
+}
+
 // CloseFile returns true if a file was opened and closed without error, or false if syscall.EBADF.
 func (c *FSContext) CloseFile(_ context.Context, fd uint32) bool {
 	f, ok := c.openedFiles[fd]


Test patch:
diff --git a/imports/wasi_snapshot_preview1/fs_test.go b/imports/wasi_snapshot_preview1/fs_test.go
index b8281923..a8b6a658 100644
--- a/imports/wasi_snapshot_preview1/fs_test.go
+++ b/imports/wasi_snapshot_preview1/fs_test.go
@@ -2567,7 +2567,8 @@ func Test_pathFilestatGet(t *testing.T) {
 			maskMemory(t, testCtx, mod, len(tc.expectedMemory))
 			mod.Memory().Write(testCtx, 0, tc.memory)
 
-			requireErrno(t, tc.expectedErrno, mod, pathFilestatGetName, uint64(tc.fd), uint64(0), uint64(1), uint64(tc.pathLen), uint64(tc.resultFilestat))
+			flags := uint32(0)
+			requireErrno(t, tc.expectedErrno, mod, pathFilestatGetName, uint64(tc.fd), uint64(flags), uint64(1), uint64(tc.pathLen), uint64(tc.resultFilestat))
 			require.Equal(t, tc.expectedLog, "\n"+log.String())
 
 			actual, ok := mod.Memory().Read(testCtx, 0, uint32(len(tc.expectedMemory)))
diff --git a/imports/wasi_snapshot_preview1/poll_test.go b/imports/wasi_snapshot_preview1/poll_test.go
index 18e2cad9..8703e49d 100644
--- a/imports/wasi_snapshot_preview1/poll_test.go
+++ b/imports/wasi_snapshot_preview1/poll_test.go
@@ -133,7 +133,7 @@ func Test_pollOneoff_Errors(t *testing.T) {
 			mem: []byte{
 				0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, // userdata
 				eventTypeFdRead, 0x0, 0x0, 0x0,
-				internalsys.FdStdin, 0x0, 0x0, 0x0, // valid readable FD
+				byte(internalsys.FdStdin), 0x0, 0x0, 0x0, // valid readable FD
 				'?', // stopped after encoding
 			},
 			expectedErrno: ErrnoSuccess,
diff --git a/imports/wasi_snapshot_preview1/wasi_bench_test.go b/imports/wasi_snapshot_preview1/wasi_bench_test.go
index 687eee73..4c278084 100644
--- a/imports/wasi_snapshot_preview1/wasi_bench_test.go
+++ b/imports/wasi_snapshot_preview1/wasi_bench_test.go
@@ -225,6 +225,96 @@ func Benchmark_fdReaddir(b *testing.B) {
 	}
 }
 
+func Benchmark_pathFilestat(b *testing.B) {
+	embedFS, err := fs.Sub(testdata, "testdata")
+	if err != nil {
+		b.Fatal(err)
+	}
+
+	benches := []struct {
+		name string
+		fs   fs.FS
+		path string
+		fd   uint32
+	}{
+		{
+			name: "embed.FS fd=root",
+			fs:   embedFS,
+			path: "zig",
+			fd:   sys.FdRoot,
+		},
+		{
+			name: "embed.FS fd=directory",
+			fs:   embedFS,
+			path:/* zig */ "wasi.zig",
+		},
+		{
+			name: "os.DirFS fd=root",
+			fs:   os.DirFS("testdata"),
+			path: "zig",
+			fd:   sys.FdRoot,
+		},
+		{
+			name: "os.DirFS fd=directory",
+			fs:   os.DirFS("testdata"),
+			path:/* zig */ "wasi.zig",
+		},
+	}
+
+	for _, bb := range benches {
+		bc := bb
+
+		b.Run(bc.name, func(b *testing.B) {
+			r := wazero.NewRuntime(testCtx)
+			defer r.Close(testCtx)
+
+			mod, err := instantiateProxyModule(r, wazero.NewModuleConfig().WithFS(bc.fs))
+			if err != nil {
+				b.Fatal(err)
+			}
+
+			// If the benchmark's file descriptor isn't root, open the file
+			// under a pre-determined directory: zig
+			fd := sys.FdRoot
+			if bc.fd != sys.FdRoot {
+				fsc := mod.(*wasm.CallContext).Sys.FS(testCtx)
+				fd, err = fsc.OpenFile(testCtx, "zig")
+				if err != nil {
+					b.Fatal(err)
+				}
+				defer fsc.CloseFile(testCtx, fd)
+			}
+
+			fn := mod.ExportedFunction(pathFilestatGetName)
+
+			b.ResetTimer()
+			b.ReportAllocs()
+			for i := 0; i < b.N; i++ {
+				b.StopTimer()
+
+				flags := uint32(0)
+				path := uint32(0)
+				pathLen := len(bc.path)
+				resultFilestat := 1024 // where to write the stat
+
+				if !mod.Memory().WriteString(testCtx, path, bc.path) {
+					b.Fatal("could not write path")
+				}
+
+				// Time the call to write the stat
+				b.StartTimer()
+				results, err := fn.Call(testCtx, uint64(fd), uint64(flags), uint64(path), uint64(pathLen), uint64(resultFilestat))
+				if err != nil {
+					b.Fatal(err)
+				}
+				b.StopTimer()
+
+				requireEsuccess(b, results)
+			}
+		})
+	}
+}
+
 func requireEsuccess(b *testing.B, results []uint64) {
 	if errno := Errno(results[0]); errno != 0 {
 		b.Fatal(ErrnoName(errno))