]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/api: make NewWatcher populate its own package and import metadata
authorBryan C. Mills <bcmills@google.com>
Fri, 20 Mar 2020 19:15:35 +0000 (15:15 -0400)
committerBryan C. Mills <bcmills@google.com>
Wed, 25 Mar 2020 19:14:38 +0000 (19:14 +0000)
This partially undoes the optimizations of CL 177597, but makes up
some of the difference by caching the package list and import metadata
and making the initial calls concurrently, including in TestMain.
That reduces the critical path from two sequential 'go list'
invocations to just one (run many times concurrently), and eliminates
the need for assumptions about the consistency of the 'std' dependency
graph across platforms (and hard-coded special cases for packages that
violate those assumptions).

In the process, this simplifies and fixes TestBenchmark (which has
been silently broken since CL 164623).

This increases 'time go tool dist test api' on my workstation from
0m8.4s / 0m13.8s / 0m1.7s to 0m10.5s / 0m23.1s / 0m5.1s,
compared to 0m12.4s / 0m23.2s / 0m4.7s before CL 177597.

(That is, this change retains about half of the wall-time speedup, but
almost none of the user-time speedup.)

Tested manually using 'go test -race -bench=. cmd/api'.

Fixes #37951

Change-Id: Icd537e035e725e1ee7c41d97da5c6651233b927e
Reviewed-on: https://go-review.googlesource.com/c/go/+/224619
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alexander Rakoczy <alex@golang.org>
src/cmd/api/goapi.go
src/cmd/api/goapi_test.go

index b46b3102678b92be6d2f4ec52fcf8dc416496871..55f3e109916d542a932609e4495419c65dfebc08 100644 (file)
@@ -26,6 +26,7 @@ import (
        "runtime"
        "sort"
        "strings"
+       "sync"
 )
 
 func goCmd() string {
@@ -138,68 +139,37 @@ func main() {
                c.Compiler = build.Default.Compiler
        }
 
-       var pkgNames []string
-       if flag.NArg() > 0 {
-               pkgNames = flag.Args()
-       } else {
-               stds, err := exec.Command(goCmd(), "list", "std").Output()
-               if err != nil {
-                       log.Fatalf("go list std: %v\n%s", err, stds)
-               }
-               for _, pkg := range strings.Fields(string(stds)) {
-                       if !internalPkg.MatchString(pkg) {
-                               pkgNames = append(pkgNames, pkg)
-                       }
-               }
+       walkers := make([]*Walker, len(contexts))
+       var wg sync.WaitGroup
+       for i, context := range contexts {
+               i, context := i, context
+               wg.Add(1)
+               go func() {
+                       defer wg.Done()
+                       walkers[i] = NewWalker(context, filepath.Join(build.Default.GOROOT, "src"))
+               }()
        }
+       wg.Wait()
 
-       importDir, importMap := loadImports()
-
-       // The code below assumes that the import map can vary
-       // by package, so that an import in one package (directory) might mean
-       // something different from the same import in another.
-       // While this can happen in GOPATH mode with vendoring,
-       // it is not possible in the standard library: the one importMap
-       // returned by loadImports applies to all packages.
-       // Construct a per-directory importMap that resolves to
-       // that single map for all packages.
-       importMapForDir := make(map[string]map[string]string)
-       for _, dir := range importDir {
-               importMapForDir[dir] = importMap
-       }
        var featureCtx = make(map[string]map[string]bool) // feature -> context name -> true
-       for _, context := range contexts {
-               w := NewWalker(context, filepath.Join(build.Default.GOROOT, "src"))
-               w.importDir = importDir
-               w.importMap = importMapForDir
+       for _, w := range walkers {
+               pkgNames := w.stdPackages
+               if flag.NArg() > 0 {
+                       pkgNames = flag.Args()
+               }
 
                for _, name := range pkgNames {
-                       // Vendored packages do not contribute to our
-                       // public API surface.
-                       if strings.HasPrefix(name, "vendor/") {
+                       pkg, err := w.Import(name)
+                       if _, nogo := err.(*build.NoGoError); nogo {
                                continue
                        }
-                       // - Package "unsafe" contains special signatures requiring
-                       //   extra care when printing them - ignore since it is not
-                       //   going to change w/o a language change.
-                       // - We don't care about the API of commands.
-                       if name != "unsafe" && !strings.HasPrefix(name, "cmd/") {
-                               if name == "runtime/cgo" && !context.CgoEnabled {
-                                       // w.Import(name) will return nil
-                                       continue
-                               }
-                               pkg, err := w.Import(name)
-                               if _, nogo := err.(*build.NoGoError); nogo {
-                                       continue
-                               }
-                               if err != nil {
-                                       log.Fatalf("Import(%q): %v", name, err)
-                               }
-                               w.export(pkg)
+                       if err != nil {
+                               log.Fatalf("Import(%q): %v", name, err)
                        }
+                       w.export(pkg)
                }
 
-               ctxName := contextName(context)
+               ctxName := contextName(w.context)
                for _, f := range w.Features() {
                        if featureCtx[f] == nil {
                                featureCtx[f] = make(map[string]bool)
@@ -368,23 +338,27 @@ func fileFeatures(filename string) []string {
 var fset = token.NewFileSet()
 
 type Walker struct {
-       context   *build.Context
-       root      string
-       scope     []string
-       current   *types.Package
-       features  map[string]bool              // set
-       imported  map[string]*types.Package    // packages already imported
-       importMap map[string]map[string]string // importer dir -> import path -> canonical path
-       importDir map[string]string            // canonical import path -> dir
+       context     *build.Context
+       root        string
+       scope       []string
+       current     *types.Package
+       features    map[string]bool              // set
+       imported    map[string]*types.Package    // packages already imported
+       stdPackages []string                     // names, omitting "unsafe", internal, and vendored packages
+       importMap   map[string]map[string]string // importer dir -> import path -> canonical path
+       importDir   map[string]string            // canonical import path -> dir
+
 }
 
 func NewWalker(context *build.Context, root string) *Walker {
-       return &Walker{
+       w := &Walker{
                context:  context,
                root:     root,
                features: map[string]bool{},
                imported: map[string]*types.Package{"unsafe": types.Unsafe},
        }
+       w.loadImports()
+       return w
 }
 
 func (w *Walker) Features() (fs []string) {
@@ -455,58 +429,112 @@ func tagKey(dir string, context *build.Context, tags []string) string {
        return key
 }
 
-// loadImports returns information about the packages in the standard library
-// and the packages they themselves import.
-// importDir maps expanded import path to the directory containing that package.
-// importMap maps source import path to expanded import path.
+type listImports struct {
+       stdPackages []string                     // names, omitting "unsafe", internal, and vendored packages
+       importDir   map[string]string            // canonical import path → directory
+       importMap   map[string]map[string]string // import path → canonical import path
+}
+
+var listCache sync.Map // map[string]listImports, keyed by contextName
+
+// loadImports populates w with information about the packages in the standard
+// library and the packages they themselves import in w's build context.
+//
 // The source import path and expanded import path are identical except for vendored packages.
 // For example, on return:
 //
-//     importMap["math"] = "math"
-//     importDir["math"] = "<goroot>/src/math"
+//     w.importMap["math"] = "math"
+//     w.importDir["math"] = "<goroot>/src/math"
 //
-//     importMap["golang.org/x/net/route"] = "vendor/golang.org/x/net/route"
-//     importDir["vendor/golang.org/x/net/route"] = "<goroot>/src/vendor/golang.org/x/net/route"
+//     w.importMap["golang.org/x/net/route"] = "vendor/golang.org/x/net/route"
+//     w.importDir["vendor/golang.org/x/net/route"] = "<goroot>/src/vendor/golang.org/x/net/route"
 //
-// There are a few imports that only appear on certain platforms,
-// including it turns out x/net/route, and we add those explicitly.
-func loadImports() (importDir map[string]string, importMap map[string]string) {
-       out, err := exec.Command(goCmd(), "list", "-e", "-deps", "-json", "std").CombinedOutput()
-       if err != nil {
-               log.Fatalf("loading imports: %v\n%s", err, out)
+// Since the set of packages that exist depends on context, the result of
+// loadImports also depends on context. However, to improve test running time
+// the configuration for each environment is cached across runs.
+func (w *Walker) loadImports() {
+       if w.context == nil {
+               return // test-only Walker; does not use the import map
        }
 
-       importDir = make(map[string]string)
-       importMap = make(map[string]string)
-       dec := json.NewDecoder(bytes.NewReader(out))
-       for {
-               var pkg struct {
-                       ImportPath, Dir string
-                       ImportMap       map[string]string
-               }
-               err := dec.Decode(&pkg)
-               if err == io.EOF {
-                       break
-               }
+       name := contextName(w.context)
+
+       imports, ok := listCache.Load(name)
+       if !ok {
+               cmd := exec.Command(goCmd(), "list", "-e", "-deps", "-json", "std")
+               cmd.Env = listEnv(w.context)
+               out, err := cmd.CombinedOutput()
                if err != nil {
-                       log.Fatalf("go list: invalid output: %v", err)
+                       log.Fatalf("loading imports: %v\n%s", err, out)
+               }
+
+               var stdPackages []string
+               importMap := make(map[string]map[string]string)
+               importDir := make(map[string]string)
+               dec := json.NewDecoder(bytes.NewReader(out))
+               for {
+                       var pkg struct {
+                               ImportPath, Dir string
+                               ImportMap       map[string]string
+                       }
+                       err := dec.Decode(&pkg)
+                       if err == io.EOF {
+                               break
+                       }
+                       if err != nil {
+                               log.Fatalf("go list: invalid output: %v", err)
+                       }
+
+                       // - Package "unsafe" contains special signatures requiring
+                       //   extra care when printing them - ignore since it is not
+                       //   going to change w/o a language change.
+                       // - internal and vendored packages do not contribute to our
+                       //   API surface.
+                       // - 'go list std' does not include commands, which cannot be
+                       //   imported anyway.
+                       if ip := pkg.ImportPath; ip != "unsafe" && !strings.HasPrefix(ip, "vendor/") && !internalPkg.MatchString(ip) {
+                               stdPackages = append(stdPackages, ip)
+                       }
+                       importDir[pkg.ImportPath] = pkg.Dir
+                       if len(pkg.ImportMap) > 0 {
+                               importMap[pkg.Dir] = make(map[string]string, len(pkg.ImportMap))
+                       }
+                       for k, v := range pkg.ImportMap {
+                               importMap[pkg.Dir][k] = v
+                       }
                }
 
-               importDir[pkg.ImportPath] = pkg.Dir
-               for k, v := range pkg.ImportMap {
-                       importMap[k] = v
+               sort.Strings(stdPackages)
+               imports = listImports{
+                       stdPackages: stdPackages,
+                       importMap:   importMap,
+                       importDir:   importDir,
                }
+               imports, _ = listCache.LoadOrStore(name, imports)
        }
 
-       // Fixup for vendor packages listed in args above.
-       fixup := []string{
-               "vendor/golang.org/x/net/route",
+       li := imports.(listImports)
+       w.stdPackages = li.stdPackages
+       w.importDir = li.importDir
+       w.importMap = li.importMap
+}
+
+// listEnv returns the process environment to use when invoking 'go list' for
+// the given context.
+func listEnv(c *build.Context) []string {
+       if c == nil {
+               return os.Environ()
        }
-       for _, pkg := range fixup {
-               importDir[pkg] = filepath.Join(build.Default.GOROOT, "src", pkg)
-               importMap[strings.TrimPrefix(pkg, "vendor/")] = pkg
+
+       environ := append(os.Environ(),
+               "GOOS="+c.GOOS,
+               "GOARCH="+c.GOARCH)
+       if c.CgoEnabled {
+               environ = append(environ, "CGO_ENABLED=1")
+       } else {
+               environ = append(environ, "CGO_ENABLED=0")
        }
-       return
+       return environ
 }
 
 // Importing is a sentinel taking the place in Walker.imported
@@ -538,7 +566,7 @@ func (w *Walker) ImportFrom(fromPath, fromDir string, mode types.ImportMode) (*t
                dir = filepath.Join(w.root, filepath.FromSlash(name))
        }
        if fi, err := os.Stat(dir); err != nil || !fi.IsDir() {
-               log.Fatalf("no source in tree for import %q (from import %s in %s): %v", name, fromPath, fromDir, err)
+               log.Panicf("no source in tree for import %q (from import %s in %s): %v", name, fromPath, fromDir, err)
        }
 
        context := w.context
index fc1bcc908af2f43d32c7230e24bcf45c63bf658e..282f26f708f1d472f4b1fd855f1e71eb663afd3c 100644 (file)
@@ -9,16 +9,36 @@ import (
        "flag"
        "fmt"
        "go/build"
-       "internal/testenv"
        "io/ioutil"
        "os"
-       "os/exec"
        "path/filepath"
        "sort"
        "strings"
+       "sync"
        "testing"
 )
 
+func TestMain(m *testing.M) {
+       flag.Parse()
+       for _, c := range contexts {
+               c.Compiler = build.Default.Compiler
+       }
+
+       // Warm up the import cache in parallel.
+       var wg sync.WaitGroup
+       for _, context := range contexts {
+               context := context
+               wg.Add(1)
+               go func() {
+                       defer wg.Done()
+                       _ = NewWalker(context, filepath.Join(build.Default.GOROOT, "src"))
+               }()
+       }
+       wg.Wait()
+
+       os.Exit(m.Run())
+}
+
 var (
        updateGolden = flag.Bool("updategolden", false, "update golden files")
 )
@@ -164,25 +184,12 @@ func TestSkipInternal(t *testing.T) {
 }
 
 func BenchmarkAll(b *testing.B) {
-       stds, err := exec.Command(testenv.GoToolPath(b), "list", "std").Output()
-       if err != nil {
-               b.Fatal(err)
-       }
-       b.ResetTimer()
-       pkgNames := strings.Fields(string(stds))
-
-       for _, c := range contexts {
-               c.Compiler = build.Default.Compiler
-       }
-
        for i := 0; i < b.N; i++ {
                for _, context := range contexts {
                        w := NewWalker(context, filepath.Join(build.Default.GOROOT, "src"))
-                       for _, name := range pkgNames {
-                               if name != "unsafe" && !strings.HasPrefix(name, "cmd/") && !internalPkg.MatchString(name) {
-                                       pkg, _ := w.Import(name)
-                                       w.export(pkg)
-                               }
+                       for _, name := range w.stdPackages {
+                               pkg, _ := w.Import(name)
+                               w.export(pkg)
                        }
                        w.Features()
                }
@@ -190,9 +197,6 @@ func BenchmarkAll(b *testing.B) {
 }
 
 func TestIssue21181(t *testing.T) {
-       for _, c := range contexts {
-               c.Compiler = build.Default.Compiler
-       }
        for _, context := range contexts {
                w := NewWalker(context, "testdata/src/issue21181")
                pkg, err := w.Import("p")
@@ -205,9 +209,6 @@ func TestIssue21181(t *testing.T) {
 }
 
 func TestIssue29837(t *testing.T) {
-       for _, c := range contexts {
-               c.Compiler = build.Default.Compiler
-       }
        for _, context := range contexts {
                w := NewWalker(context, "testdata/src/issue29837")
                _, err := w.Import("p")