From 0aedfc196bcf14df6325185f8992adbeb3f5a504 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Thu, 13 Apr 2023 20:52:53 -0400 Subject: [PATCH] go/types: update TestStdlib to type-check concurrently 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 TryBot-Result: Gopher Robot Run-TryBot: Robert Findley --- src/cmd/compile/internal/types2/self_test.go | 2 +- .../compile/internal/types2/stdlib_test.go | 205 +++++++++++++---- src/go/types/self_test.go | 2 +- src/go/types/stdlib_test.go | 209 ++++++++++++++---- 4 files changed, 334 insertions(+), 84 deletions(-) diff --git a/src/cmd/compile/internal/types2/self_test.go b/src/cmd/compile/internal/types2/self_test.go index e68d52db42..3c8bec1c45 100644 --- a/src/cmd/compile/internal/types2/self_test.go +++ b/src/cmd/compile/internal/types2/self_test.go @@ -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 } diff --git a/src/cmd/compile/internal/types2/stdlib_test.go b/src/cmd/compile/internal/types2/stdlib_test.go index 404e1636ae..9a03526b68 100644 --- a/src/cmd/compile/internal/types2/stdlib_test.go +++ b/src/cmd/compile/internal/types2/stdlib_test.go @@ -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 diff --git a/src/go/types/self_test.go b/src/go/types/self_test.go index a63f2b74f5..27fa75652a 100644 --- a/src/go/types/self_test.go +++ b/src/go/types/self_test.go @@ -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 } diff --git a/src/go/types/stdlib_test.go b/src/go/types/stdlib_test.go index 82f22de836..770d3bf52a 100644 --- a/src/go/types/stdlib_test.go +++ b/src/go/types/stdlib_test.go @@ -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 -- 2.50.0