From 913d05133c7fb3adfd2b1a34a47d635d8e072fa2 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Wed, 6 Jul 2022 13:21:34 -0400 Subject: [PATCH] cmd/go: avoid spurious readdir during fsys.Walk fsys.Walk is cloned from filepath.Walk, which has always handled a walk of a directory by reading the full directory before calling the callback on the directory itself. So if the callback returns fs.SkipDir, those entries are thrown away, but the expense of reading them was still incurred. (Worse, this is the expensive directory read that also calls Stat on every entry.) On machines with slow file system I/O, these reads are particularly annoying. For example, if I do go list m... there is a call to filepath.Walk that is told about $GOROOT/src/archive and responds by returning filepath.SkipDir because archive does not start with m, but it only gets the chance to do that after the archive directory has been read. (Same for all the other top-level directories.) Even something like go list github.com/foo/bar/... reads every top-level $GOPATH/src directory. When we designed filepath.WalkDir, one of the changes we made was to allow calling the callback twice for a directory: once before reading it, and then possibly again if the read produces an error (uncommon). This CL changes fsys.Walk to use that same model. None of the callbacks need changing, but now the $GOROOT/src/archive and other top-level directories won't be read when evaluating a pattern like 'm...'. For #53577. Fixes #53765. Change-Id: Idfa3b9e2cc335417bfd9d66dd584cb16f92bd12e Reviewed-on: https://go-review.googlesource.com/c/go/+/416179 Run-TryBot: Russ Cox Reviewed-by: Bryan Mills TryBot-Result: Gopher Robot --- src/cmd/go/internal/fsys/fsys.go | 24 +++++++------------ src/cmd/go/testdata/script/fsys_walk.txt | 6 +++++ .../{list_permissions.txt => list_perm.txt} | 3 +-- src/cmd/go/testdata/script/mod_perm.txt | 23 ++++++++++++++++++ 4 files changed, 38 insertions(+), 18 deletions(-) create mode 100644 src/cmd/go/testdata/script/fsys_walk.txt rename src/cmd/go/testdata/script/{list_permissions.txt => list_perm.txt} (99%) create mode 100644 src/cmd/go/testdata/script/mod_perm.txt diff --git a/src/cmd/go/internal/fsys/fsys.go b/src/cmd/go/internal/fsys/fsys.go index d96a290de5..0d7bef9112 100644 --- a/src/cmd/go/internal/fsys/fsys.go +++ b/src/cmd/go/internal/fsys/fsys.go @@ -464,28 +464,20 @@ func IsDirWithGoFiles(dir string) (bool, error) { // walk recursively descends path, calling walkFn. Copied, with some // modifications from path/filepath.walk. func walk(path string, info fs.FileInfo, walkFn filepath.WalkFunc) error { - if !info.IsDir() { - return walkFn(path, info, nil) + if err := walkFn(path, info, nil); err != nil || !info.IsDir() { + return err } - fis, readErr := ReadDir(path) - walkErr := walkFn(path, info, readErr) - // If readErr != nil, walk can't walk into this directory. - // walkErr != nil means walkFn want walk to skip this directory or stop walking. - // Therefore, if one of readErr and walkErr isn't nil, walk will return. - if readErr != nil || walkErr != nil { - // The caller's behavior is controlled by the return value, which is decided - // by walkFn. walkFn may ignore readErr and return nil. - // If walkFn returns SkipDir, it will be handled by the caller. - // So walk should return whatever walkFn returns. - return walkErr + fis, err := ReadDir(path) + if err != nil { + return walkFn(path, info, err) } for _, fi := range fis { filename := filepath.Join(path, fi.Name()) - if walkErr = walk(filename, fi, walkFn); walkErr != nil { - if !fi.IsDir() || walkErr != filepath.SkipDir { - return walkErr + if err := walk(filename, fi, walkFn); err != nil { + if !fi.IsDir() || err != filepath.SkipDir { + return err } } } diff --git a/src/cmd/go/testdata/script/fsys_walk.txt b/src/cmd/go/testdata/script/fsys_walk.txt new file mode 100644 index 0000000000..9d1a9451ff --- /dev/null +++ b/src/cmd/go/testdata/script/fsys_walk.txt @@ -0,0 +1,6 @@ +# Test that go list prefix... does not read directories not beginning with prefix. +env GODEBUG=gofsystrace=1 +go list m... +stderr mime +stderr mime[\\/]multipart +! stderr archive diff --git a/src/cmd/go/testdata/script/list_permissions.txt b/src/cmd/go/testdata/script/list_perm.txt similarity index 99% rename from src/cmd/go/testdata/script/list_permissions.txt rename to src/cmd/go/testdata/script/list_perm.txt index f65896ca14..3b850ef3cc 100644 --- a/src/cmd/go/testdata/script/list_permissions.txt +++ b/src/cmd/go/testdata/script/list_perm.txt @@ -11,12 +11,11 @@ stdout '^example.com/noread$' go list ./empty/... stderr 'matched no packages' -[root] stop # Root typically ignores file permissions. - # Make the directory ./noread unreadable, and verify that 'go list' reports an # explicit error for a pattern that should match it (rather than treating it as # equivalent to an empty directory). +[root] stop # Root typically ignores file permissions. [windows] skip # Does not have Unix-style directory permissions. [plan9] skip # Might not have Unix-style directory permissions. diff --git a/src/cmd/go/testdata/script/mod_perm.txt b/src/cmd/go/testdata/script/mod_perm.txt new file mode 100644 index 0000000000..f5382eceaf --- /dev/null +++ b/src/cmd/go/testdata/script/mod_perm.txt @@ -0,0 +1,23 @@ +# go list should work in ordinary conditions. +go list ./... +! stdout _data + +# skip in conditions where chmod 0 may not work. +# plan9 should be fine, but copied from list_perm.txt unchanged. +[root] skip +[windows] skip +[plan9] skip + +# go list should work with unreadable _data directory. +chmod 0 _data +go list ./... +! stdout _data + +-- go.mod -- +module m + +-- x.go -- +package m + +-- _data/x.go -- +package p -- 2.50.0