From e2921e188a77d0a621a7d8dce2216430fc3674b4 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Wed, 4 Sep 2013 13:15:49 -0400 Subject: [PATCH] go.tools/importer: make loading/parsing concurrent. 1. ParseFiles (in util.go) parses each file in its own goroutine. 2. (*Importer).LoadPackage asynchronously prefetches the import graph by scanning the imports of each loaded package and calling LoadPackage on each one. LoadPackage is now thread-safe and idempotent: it uses a condition variable per package; the first goroutine to request a package becomes responsible for loading it and broadcasts to the others (waiting) when it becomes ready. ssadump runs 34% faster when loading the oracle. Also, refactorings: - delete SourceLoader mechanism; just expose go/build.Context directly. - CreateSourcePackage now also returns an error directly, rather than via PackageInfo.Err, since every client wants that. R=crawshaw CC=golang-dev https://golang.org/cl/13509045 --- cmd/oracle/main.go | 3 +- cmd/ssadump/main.go | 5 +- importer/importer.go | 137 +++++++++++++++++++++++++------------- importer/source_test.go | 8 +-- importer/util.go | 84 ++++++++++++----------- oracle/oracle.go | 7 +- pointer/pointer_test.go | 9 +-- ssa/builder_test.go | 6 +- ssa/example_test.go | 14 ++-- ssa/interp/interp_test.go | 10 ++- ssa/source_test.go | 12 ++-- ssa/stdlib_test.go | 4 +- 12 files changed, 170 insertions(+), 129 deletions(-) diff --git a/cmd/oracle/main.go b/cmd/oracle/main.go index 1717a235c0..7c4dfe7b04 100644 --- a/cmd/oracle/main.go +++ b/cmd/oracle/main.go @@ -20,6 +20,7 @@ import ( "encoding/json" "flag" "fmt" + "go/build" "io" "log" "os" @@ -120,7 +121,7 @@ func main() { } // Ask the oracle. - res, err := oracle.Query(args, *modeFlag, *posFlag, ptalog, nil) + res, err := oracle.Query(args, *modeFlag, *posFlag, ptalog, &build.Default) if err != nil { fmt.Fprintln(os.Stderr, err) os.Exit(1) diff --git a/cmd/ssadump/main.go b/cmd/ssadump/main.go index 24992bd1fd..e35e3ca7b5 100644 --- a/cmd/ssadump/main.go +++ b/cmd/ssadump/main.go @@ -8,6 +8,7 @@ package main import ( "flag" "fmt" + "go/build" "log" "os" "runtime" @@ -66,7 +67,7 @@ func main() { flag.Parse() args := flag.Args() - impctx := importer.Config{Loader: importer.MakeGoBuildLoader(nil)} + impctx := importer.Config{Build: &build.Default} var debugMode bool var mode ssa.BuilderMode @@ -85,7 +86,7 @@ func main() { case 'N': mode |= ssa.NaiveForm case 'G': - impctx.Loader = nil + impctx.Build = nil case 'L': mode |= ssa.BuildSerially default: diff --git a/importer/importer.go b/importer/importer.go index 0fb5f2ff6c..6c7a1610eb 100644 --- a/importer/importer.go +++ b/importer/importer.go @@ -14,19 +14,32 @@ package importer import ( "fmt" "go/ast" + "go/build" "go/token" "os" + "strconv" + "sync" "code.google.com/p/go.tools/go/exact" "code.google.com/p/go.tools/go/types" ) -// An Importer's methods are not thread-safe. +// An Importer's methods are not thread-safe unless specified. type Importer struct { config *Config // the client configuration Fset *token.FileSet // position info for all files seen - Packages map[string]*PackageInfo // keys are import paths + Packages map[string]*PackageInfo // successfully imported packages keyed by import path errors map[string]error // cache of errors by import path + + mu sync.Mutex // guards 'packages' map + packages map[string]*pkgInfo // all attempted imported packages, keyed by import path +} + +// pkgInfo holds internal per-package information. +type pkgInfo struct { + info *PackageInfo // success info + err error // failure info + ready chan struct{} // channel close is notification of ready state } // Config specifies the configuration for the importer. @@ -38,7 +51,7 @@ type Config struct { // through to the type checker. TypeChecker types.Config - // If Loader is non-nil, it is used to satisfy imports. + // If Build is non-nil, it is used to satisfy imports. // // If it is nil, binary object files produced by the gc // compiler will be loaded instead of source code for all @@ -47,23 +60,9 @@ type Config struct { // code, so this mode will not yield a whole program. It is // intended for analyses that perform intraprocedural analysis // of a single package. - Loader SourceLoader + Build *build.Context } -// SourceLoader is the signature of a function that locates, reads and -// parses a set of source files given an import path. -// -// fset is the fileset to which the ASTs should be added. -// path is the imported path, e.g. "sync/atomic". -// -// On success, the function returns files, the set of ASTs produced, -// or the first error encountered. -// -// The MakeGoBuildLoader utility can be used to construct a -// SourceLoader based on go/build. -// -type SourceLoader func(fset *token.FileSet, path string) (files []*ast.File, err error) - // New returns a new, empty Importer using configuration options // specified by config. // @@ -71,6 +70,7 @@ func New(config *Config) *Importer { imp := &Importer{ config: config, Fset: token.NewFileSet(), + packages: make(map[string]*pkgInfo), Packages: make(map[string]*PackageInfo), errors: make(map[string]error), } @@ -91,8 +91,10 @@ func (imp *Importer) doImport(imports map[string]*types.Package, path string) (p // Load the source/binary for 'path', type-check it, construct // a PackageInfo and update our map (imp.Packages) and the // type-checker's map (imports). + // TODO(adonovan): make it the type-checker's job to + // consult/update the 'imports' map. Then we won't need it. var info *PackageInfo - if imp.config.Loader != nil { + if imp.config.Build != nil { info, err = imp.LoadPackage(path) } else { if info, ok := imp.Packages[path]; ok { @@ -123,37 +125,80 @@ func (imp *Importer) doImport(imports map[string]*types.Package, path string) (p return nil, err } -// LoadPackage loads the package of the specified import-path, +// imports returns the set of paths imported by the specified files. +func imports(p string, files []*ast.File) map[string]bool { + imports := make(map[string]bool) +outer: + for _, file := range files { + for _, decl := range file.Decls { + if decl, ok := decl.(*ast.GenDecl); ok { + if decl.Tok != token.IMPORT { + break outer // stop at the first non-import + } + for _, spec := range decl.Specs { + spec := spec.(*ast.ImportSpec) + if path, _ := strconv.Unquote(spec.Path.Value); path != "C" { + imports[path] = true + } + } + } else { + break outer // stop at the first non-import + } + } + } + return imports +} + +// LoadPackage loads the package of the specified import path, // performs type-checking, and returns the corresponding // PackageInfo. // -// Not idempotent! -// Precondition: Importer.config.Loader != nil. +// Precondition: Importer.config.Build != nil. +// Idempotent and thread-safe. +// // TODO(adonovan): fix: violated in call from CreatePackageFromArgs! -// Not thread-safe! // TODO(adonovan): rethink this API. // func (imp *Importer) LoadPackage(importPath string) (*PackageInfo, error) { - if info, ok := imp.Packages[importPath]; ok { - return info, nil // positive cache hit + if imp.config.Build == nil { + panic("Importer.LoadPackage without a go/build.Config") } - if err := imp.errors[importPath]; err != nil { - return nil, err // negative cache hit + imp.mu.Lock() + pi, ok := imp.packages[importPath] + if !ok { + // First request for this pkgInfo: + // create a new one in !ready state. + pi = &pkgInfo{ready: make(chan struct{})} + imp.packages[importPath] = pi + imp.mu.Unlock() + + // Load/parse the package. + if files, err := loadPackage(imp.config.Build, imp.Fset, importPath); err == nil { + // Kick off asynchronous I/O and parsing for the imports. + for path := range imports(importPath, files) { + go func() { imp.LoadPackage(path) }() + } + + // Type-check the package. + pi.info, pi.err = imp.CreateSourcePackage(importPath, files) + } else { + pi.err = err + } + + close(pi.ready) // enter ready state and wake up waiters + } else { + imp.mu.Unlock() + + <-pi.ready // wait for ready condition. } - if imp.config.Loader == nil { - panic("Importer.LoadPackage without a SourceLoader") + // Invariant: pi is ready. + + if pi.err != nil { + return nil, pi.err } - files, err := imp.config.Loader(imp.Fset, importPath) - if err != nil { - return nil, err - } - info := imp.CreateSourcePackage(importPath, files) - if info.Err != nil { - return nil, info.Err - } - return info, nil + return pi.info, nil } // CreateSourcePackage invokes the type-checker on files and returns a @@ -165,14 +210,10 @@ func (imp *Importer) LoadPackage(importPath string) (*PackageInfo, error) { // importPath is the full name under which this package is known, such // as appears in an import declaration. e.g. "sync/atomic". // -// The ParseFiles utility may be helpful for parsing a set of Go -// source files. +// Postcondition: for result (info, err), info.Err == err. // -// The result is always non-nil; the presence of errors is indicated -// by the PackageInfo.Err field. -// -func (imp *Importer) CreateSourcePackage(importPath string, files []*ast.File) *PackageInfo { - pkgInfo := &PackageInfo{ +func (imp *Importer) CreateSourcePackage(importPath string, files []*ast.File) (*PackageInfo, error) { + info := &PackageInfo{ Files: files, Info: types.Info{ Types: make(map[ast.Expr]types.Type), @@ -183,7 +224,7 @@ func (imp *Importer) CreateSourcePackage(importPath string, files []*ast.File) * Selections: make(map[*ast.SelectorExpr]*types.Selection), }, } - pkgInfo.Pkg, pkgInfo.Err = imp.config.TypeChecker.Check(importPath, imp.Fset, files, &pkgInfo.Info) - imp.Packages[importPath] = pkgInfo - return pkgInfo + info.Pkg, info.Err = imp.config.TypeChecker.Check(importPath, imp.Fset, files, &info.Info) + imp.Packages[importPath] = info + return info, info.Err } diff --git a/importer/source_test.go b/importer/source_test.go index 961e0565a0..509cbf6a81 100644 --- a/importer/source_test.go +++ b/importer/source_test.go @@ -239,7 +239,7 @@ func TestEnclosingFunction(t *testing.T) { "900", "func@2.27"}, } for _, test := range tests { - imp := importer.New(new(importer.Config)) // (NB: no Loader) + imp := importer.New(new(importer.Config)) // (NB: no go/build.Config) f, start, end := findInterval(t, imp.Fset, test.input, test.substr) if f == nil { continue @@ -249,9 +249,9 @@ func TestEnclosingFunction(t *testing.T) { t.Errorf("EnclosingFunction(%q) not exact", test.substr) continue } - info := imp.CreateSourcePackage("main", []*ast.File{f}) - if info.Err != nil { - t.Error(info.Err.Error()) + info, err := imp.CreateSourcePackage("main", []*ast.File{f}) + if err != nil { + t.Error(err.Error()) continue } diff --git a/importer/util.go b/importer/util.go index 1c260dc974..44485cbd31 100644 --- a/importer/util.go +++ b/importer/util.go @@ -16,6 +16,7 @@ import ( "os" "path/filepath" "strings" + "sync" ) // CreatePackageFromArgs builds an initial Package from a list of @@ -30,7 +31,7 @@ import ( func CreatePackageFromArgs(imp *Importer, args []string) (info *PackageInfo, rest []string, err error) { switch { case len(args) == 0: - return nil, nil, errors.New("No *.go source files nor package name was specified.") + return nil, nil, errors.New("no *.go source files nor package name was specified.") case strings.HasSuffix(args[0], ".go"): // % tool a.go b.go ... @@ -42,8 +43,7 @@ func CreatePackageFromArgs(imp *Importer, args []string) (info *PackageInfo, res files, err = ParseFiles(imp.Fset, ".", args[:i]...) rest = args[i:] if err == nil { - info = imp.CreateSourcePackage("main", files) - err = info.Err + info, err = imp.CreateSourcePackage("main", files) } default: @@ -57,59 +57,57 @@ func CreatePackageFromArgs(imp *Importer, args []string) (info *PackageInfo, res return } -// MakeGoBuildLoader returns an implementation of the SourceLoader -// function prototype that locates packages using the go/build -// libraries. It may return nil upon gross misconfiguration -// (e.g. os.Getwd() failed). -// -// ctxt specifies the go/build.Context to use; if nil, the default -// Context is used. -// -func MakeGoBuildLoader(ctxt *build.Context) SourceLoader { - srcDir, err := os.Getwd() +var cwd string + +func init() { + var err error + cwd, err = os.Getwd() if err != nil { - return nil // serious misconfiguration + panic("getcwd failed: " + err.Error()) } - if ctxt == nil { - ctxt = &build.Default +} + +// loadPackage ascertains which files belong to package path, then +// loads, parses and returns them. +func loadPackage(ctxt *build.Context, fset *token.FileSet, path string) (files []*ast.File, err error) { + // TODO(adonovan): fix: Do we need cwd? Shouldn't + // ImportDir(path) / $GOROOT suffice? + bp, err := ctxt.Import(path, cwd, 0) + if _, ok := err.(*build.NoGoError); ok { + return nil, nil // empty directory } - return func(fset *token.FileSet, path string) (files []*ast.File, err error) { - // TODO(adonovan): fix: Do we need cwd? Shouldn't - // ImportDir(path) / $GOROOT suffice? - bp, err := ctxt.Import(path, srcDir, 0) - if _, ok := err.(*build.NoGoError); ok { - return nil, nil // empty directory - } - if err != nil { - return // import failed - } - files, err = ParseFiles(fset, bp.Dir, bp.GoFiles...) - if err != nil { - return nil, err - } - return + if err != nil { + return // import failed } + return ParseFiles(fset, bp.Dir, bp.GoFiles...) } // ParseFiles parses the Go source files files within directory dir // and returns their ASTs, or the first parse error if any. // -// This utility function is provided to facilitate implementing a -// SourceLoader. -// -func ParseFiles(fset *token.FileSet, dir string, files ...string) (parsed []*ast.File, err error) { - for _, file := range files { - var f *ast.File +func ParseFiles(fset *token.FileSet, dir string, files ...string) ([]*ast.File, error) { + var wg sync.WaitGroup + n := len(files) + parsed := make([]*ast.File, n, n) + errors := make([]error, n, n) + for i, file := range files { if !filepath.IsAbs(file) { file = filepath.Join(dir, file) } - f, err = parser.ParseFile(fset, file, nil, parser.DeclarationErrors) - if err != nil { - return // parsing failed - } - parsed = append(parsed, f) + wg.Add(1) + go func(i int, file string) { + parsed[i], errors[i] = parser.ParseFile(fset, file, nil, parser.DeclarationErrors) + wg.Done() + }(i, file) } - return + wg.Wait() + + for _, err := range errors { + if err != nil { + return nil, err + } + } + return parsed, nil } // ---------- Internal helpers ---------- diff --git a/oracle/oracle.go b/oracle/oracle.go index 2a5599d43b..d54fb2d0e7 100644 --- a/oracle/oracle.go +++ b/oracle/oracle.go @@ -128,11 +128,10 @@ func Query(args []string, mode, pos string, ptalog io.Writer, buildContext *buil return nil, fmt.Errorf("invalid mode type: %q", mode) } - var loader importer.SourceLoader - if minfo.needs&WholeSource != 0 { - loader = importer.MakeGoBuildLoader(buildContext) + if minfo.needs&WholeSource == 0 { + buildContext = nil } - imp := importer.New(&importer.Config{Loader: loader}) + imp := importer.New(&importer.Config{Build: buildContext}) o := &oracle{ prog: ssa.NewProgram(imp.Fset, 0), timers: make(map[string]time.Duration), diff --git a/pointer/pointer_test.go b/pointer/pointer_test.go index b99694893e..c05b504da9 100644 --- a/pointer/pointer_test.go +++ b/pointer/pointer_test.go @@ -12,6 +12,7 @@ import ( "bytes" "fmt" "go/ast" + "go/build" "go/parser" "go/token" "io/ioutil" @@ -162,7 +163,7 @@ func findProbe(prog *ssa.Program, probes []probe, e *expectation) *probe { } func doOneInput(input, filename string) bool { - impctx := &importer.Config{Loader: importer.MakeGoBuildLoader(nil)} + impctx := &importer.Config{Build: &build.Default} imp := importer.New(impctx) // Parsing. @@ -175,9 +176,9 @@ func doOneInput(input, filename string) bool { } // Type checking. - info := imp.CreateSourcePackage("main", []*ast.File{f}) - if info.Err != nil { - fmt.Println(info.Err.Error()) + info, err := imp.CreateSourcePackage("main", []*ast.File{f}) + if err != nil { + fmt.Println(err.Error()) return false } diff --git a/ssa/builder_test.go b/ssa/builder_test.go index 138b24c06e..72f58f3057 100644 --- a/ssa/builder_test.go +++ b/ssa/builder_test.go @@ -46,9 +46,9 @@ func main() { return } - info := imp.CreateSourcePackage("main", []*ast.File{f}) - if info.Err != nil { - t.Error(info.Err.Error()) + info, err := imp.CreateSourcePackage("main", []*ast.File{f}) + if err != nil { + t.Error(err.Error()) return } diff --git a/ssa/example_test.go b/ssa/example_test.go index 2803a6d7a9..b915a366d5 100644 --- a/ssa/example_test.go +++ b/ssa/example_test.go @@ -5,12 +5,14 @@ package ssa_test import ( - "code.google.com/p/go.tools/importer" - "code.google.com/p/go.tools/ssa" "fmt" "go/ast" + "go/build" "go/parser" "os" + + "code.google.com/p/go.tools/importer" + "code.google.com/p/go.tools/ssa" ) // This program demonstrates how to run the SSA builder on a "Hello, @@ -41,7 +43,7 @@ func main() { } ` // Construct an importer. Imports will be loaded as if by 'go build'. - imp := importer.New(&importer.Config{Loader: importer.MakeGoBuildLoader(nil)}) + imp := importer.New(&importer.Config{Build: &build.Default}) // Parse the input file. file, err := parser.ParseFile(imp.Fset, "hello.go", hello, parser.DeclarationErrors) @@ -51,9 +53,9 @@ func main() { } // Create a "main" package containing one file. - info := imp.CreateSourcePackage("main", []*ast.File{file}) - if info.Err != nil { - fmt.Print(info.Err.Error()) // type error + info, err := imp.CreateSourcePackage("main", []*ast.File{file}) + if err != nil { + fmt.Print(err.Error()) // type error return } diff --git a/ssa/interp/interp_test.go b/ssa/interp/interp_test.go index 9c0a55ce74..69250ef48a 100644 --- a/ssa/interp/interp_test.go +++ b/ssa/interp/interp_test.go @@ -165,9 +165,7 @@ func run(t *testing.T, dir, input string) bool { inputs = append(inputs, dir+i) } - imp := importer.New(&importer.Config{ - Loader: importer.MakeGoBuildLoader(nil), - }) + imp := importer.New(&importer.Config{Build: &build.Default}) files, err := importer.ParseFiles(imp.Fset, ".", inputs...) if err != nil { t.Errorf("ssa.ParseFiles(%s) failed: %s", inputs, err.Error()) @@ -186,9 +184,9 @@ func run(t *testing.T, dir, input string) bool { }() hint = fmt.Sprintf("To dump SSA representation, run:\n%% go build code.google.com/p/go.tools/cmd/ssadump; ./ssadump -build=CFP %s\n", input) - info := imp.CreateSourcePackage("main", files) - if info.Err != nil { - t.Errorf("importer.CreateSourcePackage(%s) failed: %s", inputs, info.Err.Error()) + info, err := imp.CreateSourcePackage("main", files) + if err != nil { + t.Errorf("importer.CreateSourcePackage(%s) failed: %s", inputs, err) return false } diff --git a/ssa/source_test.go b/ssa/source_test.go index 6e27c4541e..7dd52ccc8f 100644 --- a/ssa/source_test.go +++ b/ssa/source_test.go @@ -46,9 +46,9 @@ func TestObjValueLookup(t *testing.T) { } } - info := imp.CreateSourcePackage("main", []*ast.File{f}) - if info.Err != nil { - t.Error(info.Err.Error()) + info, err := imp.CreateSourcePackage("main", []*ast.File{f}) + if err != nil { + t.Error(err.Error()) return } @@ -197,9 +197,9 @@ func TestValueForExpr(t *testing.T) { return } - info := imp.CreateSourcePackage("main", []*ast.File{f}) - if info.Err != nil { - t.Error(info.Err.Error()) + info, err := imp.CreateSourcePackage("main", []*ast.File{f}) + if err != nil { + t.Error(err.Error()) return } diff --git a/ssa/stdlib_test.go b/ssa/stdlib_test.go index 8bfcdeafb6..ea741b1fba 100644 --- a/ssa/stdlib_test.go +++ b/ssa/stdlib_test.go @@ -53,8 +53,8 @@ func allPackages() []string { func TestStdlib(t *testing.T) { ctxt := build.Default - ctxt.CgoEnabled = false - impctx := importer.Config{Loader: importer.MakeGoBuildLoader(&ctxt)} + ctxt.CgoEnabled = false // mutating a global! + impctx := importer.Config{Build: &ctxt} // Load, parse and type-check the program. t0 := time.Now()