]> Cypherpunks repositories - gostls13.git/commitdiff
go/types: update TestStdlib to type-check concurrently
authorRob Findley <rfindley@google.com>
Fri, 14 Apr 2023 00:52:53 +0000 (20:52 -0400)
committerRobert Findley <rfindley@google.com>
Tue, 23 May 2023 19:39:00 +0000 (19:39 +0000)
In order to have some test coverage of concurrent use of the go/types
APIs, update the Stdlib test to type-check concurrently. In combination
with non-deterministic ordering, this should hopefully provide moderate
test coverage of concurrent use.

Also, remove the arbitrary 10ms timeout in short mode, in favor of
simply not running.

After this change, TestStdlib went from taking 16s on my laptop to 2s,
in part because of the parallelism and in part because we are no longer
type-checking twice (once for the import er, once for the test).

Fixes golang/go#47729

Change-Id: Ie49743947ab2d5aec051c3d09ce045acf5b94ad4
Reviewed-on: https://go-review.googlesource.com/c/go/+/484540
Reviewed-by: Robert Griesemer <gri@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>

src/cmd/compile/internal/types2/self_test.go
src/cmd/compile/internal/types2/stdlib_test.go
src/go/types/self_test.go
src/go/types/stdlib_test.go

index e68d52db42e2bd291bcb75b6dcff55a2fefab4e3..3c8bec1c45a458c1de810fea2abf837574c66695 100644 (file)
@@ -100,7 +100,7 @@ func runbench(b *testing.B, path string, ignoreFuncBodies, writeInfo bool) {
 }
 
 func pkgFiles(path string) ([]*syntax.File, error) {
-       filenames, err := pkgFilenames(path) // from stdlib_test.go
+       filenames, err := pkgFilenames(path, true) // from stdlib_test.go
        if err != nil {
                return nil, err
        }
index 404e1636aec480bf9f97607903fb441c58beefde..9a03526b68ba4d17b1fbb5249ad0efe062eafcbc 100644 (file)
@@ -10,12 +10,15 @@ package types2_test
 import (
        "bytes"
        "cmd/compile/internal/syntax"
+       "errors"
        "fmt"
        "go/build"
        "internal/testenv"
        "os"
        "path/filepath"
+       "runtime"
        "strings"
+       "sync"
        "testing"
        "time"
 
@@ -25,17 +28,130 @@ import (
 var stdLibImporter = defaultImporter()
 
 func TestStdlib(t *testing.T) {
+       if testing.Short() {
+               t.Skip("skipping in short mode")
+       }
+
        testenv.MustHaveGoBuild(t)
 
-       pkgCount := 0
-       duration := walkPkgDirs(filepath.Join(testenv.GOROOT(t), "src"), func(dir string, filenames []string) {
-               typecheckFiles(t, dir, filenames)
-               pkgCount++
+       // Collect non-test files.
+       dirFiles := make(map[string][]string)
+       root := filepath.Join(testenv.GOROOT(t), "src")
+       walkPkgDirs(root, func(dir string, filenames []string) {
+               dirFiles[dir] = filenames
        }, t.Error)
 
+       c := &stdlibChecker{
+               dirFiles: dirFiles,
+               pkgs:     make(map[string]*futurePackage),
+       }
+
+       start := time.Now()
+
+       // Though we read files while parsing, type-checking is otherwise CPU bound.
+       //
+       // This doesn't achieve great CPU utilization as many packages may block
+       // waiting for a common import, but in combination with the non-deterministic
+       // map iteration below this should provide decent coverage of concurrent
+       // type-checking (see golang/go#47729).
+       cpulimit := make(chan struct{}, runtime.GOMAXPROCS(0))
+       var wg sync.WaitGroup
+
+       for dir := range dirFiles {
+               dir := dir
+
+               cpulimit <- struct{}{}
+               wg.Add(1)
+               go func() {
+                       defer func() {
+                               wg.Done()
+                               <-cpulimit
+                       }()
+
+                       _, err := c.getDirPackage(dir)
+                       if err != nil {
+                               t.Errorf("error checking %s: %v", dir, err)
+                       }
+               }()
+       }
+
+       wg.Wait()
+
        if testing.Verbose() {
-               fmt.Println(pkgCount, "packages typechecked in", duration)
+               fmt.Println(len(dirFiles), "packages typechecked in", time.Since(start))
+       }
+}
+
+// stdlibChecker implements concurrent type-checking of the packages defined by
+// dirFiles, which must define a closed set of packages (such as GOROOT/src).
+type stdlibChecker struct {
+       dirFiles map[string][]string // non-test files per directory; must be pre-populated
+
+       mu   sync.Mutex
+       pkgs map[string]*futurePackage // future cache of type-checking results
+}
+
+// A futurePackage is a future result of type-checking.
+type futurePackage struct {
+       done chan struct{} // guards pkg and err
+       pkg  *Package
+       err  error
+}
+
+func (c *stdlibChecker) Import(path string) (*Package, error) {
+       panic("unimplemented: use ImportFrom")
+}
+
+func (c *stdlibChecker) ImportFrom(path, dir string, _ ImportMode) (*Package, error) {
+       if path == "unsafe" {
+               // unsafe cannot be type checked normally.
+               return Unsafe, nil
+       }
+
+       p, err := build.Default.Import(path, dir, build.FindOnly)
+       if err != nil {
+               return nil, err
+       }
+
+       pkg, err := c.getDirPackage(p.Dir)
+       if pkg != nil {
+               // As long as pkg is non-nil, avoid redundant errors related to failed
+               // imports. TestStdlib will collect errors once for each package.
+               return pkg, nil
+       }
+       return nil, err
+}
+
+// getDirPackage gets the package defined in dir from the future cache.
+//
+// If this is the first goroutine requesting the package, getDirPackage
+// type-checks.
+func (c *stdlibChecker) getDirPackage(dir string) (*Package, error) {
+       c.mu.Lock()
+       fut, ok := c.pkgs[dir]
+       if !ok {
+               // First request for this package dir; type check.
+               fut = &futurePackage{
+                       done: make(chan struct{}),
+               }
+               c.pkgs[dir] = fut
+               files, ok := c.dirFiles[dir]
+               c.mu.Unlock()
+               if !ok {
+                       fut.err = fmt.Errorf("no files for %s", dir)
+               } else {
+                       // Using dir as the package path here may be inconsistent with the behavior
+                       // of a normal importer, but is sufficient as dir is by construction unique
+                       // to this package.
+                       fut.pkg, fut.err = typecheckFiles(dir, files, c)
+               }
+               close(fut.done)
+       } else {
+               // Otherwise, await the result.
+               c.mu.Unlock()
+               <-fut.done
        }
+       return fut.pkg, fut.err
 }
 
 // firstComment returns the contents of the first non-empty comment in
@@ -230,34 +346,51 @@ var excluded = map[string]bool{
        "crypto/internal/bigmod/_asm":             true,
 }
 
+// printPackageMu synchronizes the printing of type-checked package files in
+// the typecheckFiles function.
+//
+// Without synchronization, package files may be interleaved during concurrent
+// type-checking.
+var printPackageMu sync.Mutex
+
 // typecheckFiles typechecks the given package files.
-func typecheckFiles(t *testing.T, path string, filenames []string) {
-       // parse package files
+func typecheckFiles(path string, filenames []string, importer Importer) (*Package, error) {
+       // Parse package files.
        var files []*syntax.File
        for _, filename := range filenames {
-               errh := func(err error) { t.Error(err) }
+               var errs []error
+               errh := func(err error) { errs = append(errs, err) }
                file, err := syntax.ParseFile(filename, errh, nil, 0)
                if err != nil {
-                       return
+                       return nil, errors.Join(errs...)
                }
 
-               if testing.Verbose() {
-                       if len(files) == 0 {
-                               fmt.Println("package", file.PkgName.Value)
-                       }
+               files = append(files, file)
+       }
+
+       if testing.Verbose() {
+               printPackageMu.Lock()
+               fmt.Println("package", files[0].PkgName.Value)
+               for _, filename := range filenames {
                        fmt.Println("\t", filename)
                }
-
-               files = append(files, file)
+               printPackageMu.Unlock()
        }
 
-       // typecheck package files
+       // Typecheck package files.
+       var errs []error
        conf := Config{
-               Error:    func(err error) { t.Error(err) },
-               Importer: stdLibImporter,
+               Error: func(err error) {
+                       errs = append(errs, err)
+               },
+               Importer: importer,
        }
        info := Info{Uses: make(map[*syntax.Name]Object)}
-       conf.Check(path, files, &info)
+       pkg, _ := conf.Check(path, files, &info)
+       err := errors.Join(errs...)
+       if err != nil {
+               return pkg, err
+       }
 
        // Perform checks of API invariants.
 
@@ -268,16 +401,18 @@ func typecheckFiles(t *testing.T, path string, filenames []string) {
                if predeclared == (obj.Pkg() != nil) {
                        posn := id.Pos()
                        if predeclared {
-                               t.Errorf("%s: predeclared object with package: %s", posn, obj)
+                               return nil, fmt.Errorf("%s: predeclared object with package: %s", posn, obj)
                        } else {
-                               t.Errorf("%s: user-defined object without package: %s", posn, obj)
+                               return nil, fmt.Errorf("%s: user-defined object without package: %s", posn, obj)
                        }
                }
        }
+
+       return pkg, nil
 }
 
 // pkgFilenames returns the list of package filenames for the given directory.
-func pkgFilenames(dir string) ([]string, error) {
+func pkgFilenames(dir string, includeTest bool) ([]string, error) {
        ctxt := build.Default
        ctxt.CgoEnabled = false
        pkg, err := ctxt.ImportDir(dir, 0)
@@ -294,31 +429,25 @@ func pkgFilenames(dir string) ([]string, error) {
        for _, name := range pkg.GoFiles {
                filenames = append(filenames, filepath.Join(pkg.Dir, name))
        }
-       for _, name := range pkg.TestGoFiles {
-               filenames = append(filenames, filepath.Join(pkg.Dir, name))
+       if includeTest {
+               for _, name := range pkg.TestGoFiles {
+                       filenames = append(filenames, filepath.Join(pkg.Dir, name))
+               }
        }
        return filenames, nil
 }
 
-func walkPkgDirs(dir string, pkgh func(dir string, filenames []string), errh func(args ...interface{})) time.Duration {
-       w := walker{time.Now(), 10 * time.Millisecond, pkgh, errh}
+func walkPkgDirs(dir string, pkgh func(dir string, filenames []string), errh func(args ...interface{})) {
+       w := walker{pkgh, errh}
        w.walk(dir)
-       return time.Since(w.start)
 }
 
 type walker struct {
-       start time.Time
-       dmax  time.Duration
-       pkgh  func(dir string, filenames []string)
-       errh  func(args ...interface{})
+       pkgh func(dir string, filenames []string)
+       errh func(args ...any)
 }
 
 func (w *walker) walk(dir string) {
-       // limit run time for short tests
-       if testing.Short() && time.Since(w.start) >= w.dmax {
-               return
-       }
-
        files, err := os.ReadDir(dir)
        if err != nil {
                w.errh(err)
@@ -326,7 +455,9 @@ func (w *walker) walk(dir string) {
        }
 
        // apply pkgh to the files in directory dir
-       pkgFiles, err := pkgFilenames(dir)
+
+       // Don't get test files as these packages are imported.
+       pkgFiles, err := pkgFilenames(dir, false)
        if err != nil {
                w.errh(err)
                return
index a63f2b74f5f96d251bd7da22778719f3fcf286f8..27fa75652a5204a3663a2eb89311a442416a1898 100644 (file)
@@ -104,7 +104,7 @@ func runbench(b *testing.B, path string, ignoreFuncBodies, writeInfo bool) {
 }
 
 func pkgFiles(fset *token.FileSet, path string) ([]*ast.File, error) {
-       filenames, err := pkgFilenames(path) // from stdlib_test.go
+       filenames, err := pkgFilenames(path, true) // from stdlib_test.go
        if err != nil {
                return nil, err
        }
index 82f22de8362d7d0b23c69ba9847a2def2be6d379..770d3bf52abeb5f6093aea7fdd7eff2a09923d80 100644 (file)
@@ -8,6 +8,7 @@
 package types_test
 
 import (
+       "errors"
        "fmt"
        "go/ast"
        "go/build"
@@ -18,7 +19,9 @@ import (
        "internal/testenv"
        "os"
        "path/filepath"
+       "runtime"
        "strings"
+       "sync"
        "testing"
        "time"
 
@@ -36,19 +39,132 @@ import (
 var stdLibImporter = importer.ForCompiler(token.NewFileSet(), "source", nil)
 
 func TestStdlib(t *testing.T) {
+       if testing.Short() {
+               t.Skip("skipping in short mode")
+       }
+
        testenv.MustHaveGoBuild(t)
 
-       pkgCount := 0
-       duration := walkPkgDirs(filepath.Join(testenv.GOROOT(t), "src"), func(dir string, filenames []string) {
-               typecheckFiles(t, dir, filenames)
-               pkgCount++
+       // Collect non-test files.
+       dirFiles := make(map[string][]string)
+       root := filepath.Join(testenv.GOROOT(t), "src")
+       walkPkgDirs(root, func(dir string, filenames []string) {
+               dirFiles[dir] = filenames
        }, t.Error)
 
+       c := &stdlibChecker{
+               dirFiles: dirFiles,
+               pkgs:     make(map[string]*futurePackage),
+       }
+
+       start := time.Now()
+
+       // Though we read files while parsing, type-checking is otherwise CPU bound.
+       //
+       // This doesn't achieve great CPU utilization as many packages may block
+       // waiting for a common import, but in combination with the non-deterministic
+       // map iteration below this should provide decent coverage of concurrent
+       // type-checking (see golang/go#47729).
+       cpulimit := make(chan struct{}, runtime.GOMAXPROCS(0))
+       var wg sync.WaitGroup
+
+       for dir := range dirFiles {
+               dir := dir
+
+               cpulimit <- struct{}{}
+               wg.Add(1)
+               go func() {
+                       defer func() {
+                               wg.Done()
+                               <-cpulimit
+                       }()
+
+                       _, err := c.getDirPackage(dir)
+                       if err != nil {
+                               t.Errorf("error checking %s: %v", dir, err)
+                       }
+               }()
+       }
+
+       wg.Wait()
+
        if testing.Verbose() {
-               fmt.Println(pkgCount, "packages typechecked in", duration)
+               fmt.Println(len(dirFiles), "packages typechecked in", time.Since(start))
        }
 }
 
+// stdlibChecker implements concurrent type-checking of the packages defined by
+// dirFiles, which must define a closed set of packages (such as GOROOT/src).
+type stdlibChecker struct {
+       dirFiles map[string][]string // non-test files per directory; must be pre-populated
+
+       mu   sync.Mutex
+       pkgs map[string]*futurePackage // future cache of type-checking results
+}
+
+// A futurePackage is a future result of type-checking.
+type futurePackage struct {
+       done chan struct{} // guards pkg and err
+       pkg  *Package
+       err  error
+}
+
+func (c *stdlibChecker) Import(path string) (*Package, error) {
+       panic("unimplemented: use ImportFrom")
+}
+
+func (c *stdlibChecker) ImportFrom(path, dir string, _ ImportMode) (*Package, error) {
+       if path == "unsafe" {
+               // unsafe cannot be type checked normally.
+               return Unsafe, nil
+       }
+
+       p, err := build.Default.Import(path, dir, build.FindOnly)
+       if err != nil {
+               return nil, err
+       }
+
+       pkg, err := c.getDirPackage(p.Dir)
+       if pkg != nil {
+               // As long as pkg is non-nil, avoid redundant errors related to failed
+               // imports. TestStdlib will collect errors once for each package.
+               return pkg, nil
+       }
+       return nil, err
+}
+
+// getDirPackage gets the package defined in dir from the future cache.
+//
+// If this is the first goroutine requesting the package, getDirPackage
+// type-checks.
+func (c *stdlibChecker) getDirPackage(dir string) (*Package, error) {
+       c.mu.Lock()
+       fut, ok := c.pkgs[dir]
+       if !ok {
+               // First request for this package dir; type check.
+               fut = &futurePackage{
+                       done: make(chan struct{}),
+               }
+               c.pkgs[dir] = fut
+               files, ok := c.dirFiles[dir]
+               c.mu.Unlock()
+               if !ok {
+                       fut.err = fmt.Errorf("no files for %s", dir)
+               } else {
+                       // Using dir as the package path here may be inconsistent with the behavior
+                       // of a normal importer, but is sufficient as dir is by construction unique
+                       // to this package.
+                       fut.pkg, fut.err = typecheckFiles(dir, files, c)
+               }
+               close(fut.done)
+       } else {
+               // Otherwise, await the result.
+               c.mu.Unlock()
+               <-fut.done
+       }
+       return fut.pkg, fut.err
+}
+
 // firstComment returns the contents of the first non-empty comment in
 // the given file, "skip", or the empty string. No matter the present
 // comments, if any of them contains a build tag, the result is always
@@ -232,46 +348,51 @@ var excluded = map[string]bool{
        "crypto/internal/bigmod/_asm":             true,
 }
 
+// printPackageMu synchronizes the printing of type-checked package files in
+// the typecheckFiles function.
+//
+// Without synchronization, package files may be interleaved during concurrent
+// type-checking.
+var printPackageMu sync.Mutex
+
 // typecheckFiles typechecks the given package files.
-func typecheckFiles(t *testing.T, path string, filenames []string) {
+func typecheckFiles(path string, filenames []string, importer Importer) (*Package, error) {
        fset := token.NewFileSet()
 
-       // parse package files
+       // Parse package files.
        var files []*ast.File
        for _, filename := range filenames {
                file, err := parser.ParseFile(fset, filename, nil, parser.AllErrors)
                if err != nil {
-                       // the parser error may be a list of individual errors; report them all
-                       if list, ok := err.(scanner.ErrorList); ok {
-                               for _, err := range list {
-                                       t.Error(err)
-                               }
-                               return
-                       }
-                       t.Error(err)
-                       return
+                       return nil, err
                }
 
-               if testing.Verbose() {
-                       if len(files) == 0 {
-                               fmt.Println("package", file.Name.Name)
-                       }
+               files = append(files, file)
+       }
+
+       if testing.Verbose() {
+               printPackageMu.Lock()
+               fmt.Println("package", files[0].Name.Name)
+               for _, filename := range filenames {
                        fmt.Println("\t", filename)
                }
-
-               files = append(files, file)
+               printPackageMu.Unlock()
        }
 
-       // typecheck package files
+       // Typecheck package files.
+       var errs []error
        conf := Config{
                Error: func(err error) {
-                       t.Helper()
-                       t.Error(err)
+                       errs = append(errs, err)
                },
-               Importer: stdLibImporter,
+               Importer: importer,
        }
        info := Info{Uses: make(map[*ast.Ident]Object)}
-       conf.Check(path, fset, files, &info)
+       pkg, _ := conf.Check(path, fset, files, &info)
+       err := errors.Join(errs...)
+       if err != nil {
+               return pkg, err
+       }
 
        // Perform checks of API invariants.
 
@@ -282,16 +403,18 @@ func typecheckFiles(t *testing.T, path string, filenames []string) {
                if predeclared == (obj.Pkg() != nil) {
                        posn := fset.Position(id.Pos())
                        if predeclared {
-                               t.Errorf("%s: predeclared object with package: %s", posn, obj)
+                               return nil, fmt.Errorf("%s: predeclared object with package: %s", posn, obj)
                        } else {
-                               t.Errorf("%s: user-defined object without package: %s", posn, obj)
+                               return nil, fmt.Errorf("%s: user-defined object without package: %s", posn, obj)
                        }
                }
        }
+
+       return pkg, nil
 }
 
 // pkgFilenames returns the list of package filenames for the given directory.
-func pkgFilenames(dir string) ([]string, error) {
+func pkgFilenames(dir string, includeTest bool) ([]string, error) {
        ctxt := build.Default
        ctxt.CgoEnabled = false
        pkg, err := ctxt.ImportDir(dir, 0)
@@ -308,31 +431,25 @@ func pkgFilenames(dir string) ([]string, error) {
        for _, name := range pkg.GoFiles {
                filenames = append(filenames, filepath.Join(pkg.Dir, name))
        }
-       for _, name := range pkg.TestGoFiles {
-               filenames = append(filenames, filepath.Join(pkg.Dir, name))
+       if includeTest {
+               for _, name := range pkg.TestGoFiles {
+                       filenames = append(filenames, filepath.Join(pkg.Dir, name))
+               }
        }
        return filenames, nil
 }
 
-func walkPkgDirs(dir string, pkgh func(dir string, filenames []string), errh func(args ...any)) time.Duration {
-       w := walker{time.Now(), 10 * time.Millisecond, pkgh, errh}
+func walkPkgDirs(dir string, pkgh func(dir string, filenames []string), errh func(args ...any)) {
+       w := walker{pkgh, errh}
        w.walk(dir)
-       return time.Since(w.start)
 }
 
 type walker struct {
-       start time.Time
-       dmax  time.Duration
-       pkgh  func(dir string, filenames []string)
-       errh  func(args ...any)
+       pkgh func(dir string, filenames []string)
+       errh func(args ...any)
 }
 
 func (w *walker) walk(dir string) {
-       // limit run time for short tests
-       if testing.Short() && time.Since(w.start) >= w.dmax {
-               return
-       }
-
        files, err := os.ReadDir(dir)
        if err != nil {
                w.errh(err)
@@ -340,7 +457,9 @@ func (w *walker) walk(dir string) {
        }
 
        // apply pkgh to the files in directory dir
-       pkgFiles, err := pkgFilenames(dir)
+
+       // Don't get test files as these packages are imported.
+       pkgFiles, err := pkgFilenames(dir, false)
        if err != nil {
                w.errh(err)
                return