From 48599c64ba9505e7fd34ddc3e21f3785baaa2c6a Mon Sep 17 00:00:00 2001 From: Isaev Denis Date: Mon, 14 Oct 2019 09:50:57 +0300 Subject: [PATCH] Make fine-grained hashing. (#814) Speed up golint: don't typecheck packages twice. Relates: #805 --- go.mod | 2 +- go.sum | 4 +- internal/pkgcache/pkgcache.go | 87 ++++++++++++++++------- pkg/exitcodes/exitcodes.go | 2 + pkg/golinters/goanalysis/linter.go | 14 ++-- pkg/golinters/goanalysis/runner.go | 15 ++-- pkg/golinters/golint.go | 10 +-- pkg/lint/lintersdb/manager.go | 1 + pkg/timeutils/stopwatch.go | 42 +++++++++-- vendor/github.com/golangci/lint-1/lint.go | 27 ++----- vendor/modules.txt | 2 +- 11 files changed, 138 insertions(+), 68 deletions(-) diff --git a/go.mod b/go.mod index b6fe6106..9e5b2f67 100644 --- a/go.mod +++ b/go.mod @@ -17,7 +17,7 @@ require ( github.com/golangci/gocyclo v0.0.0-20180528134321-2becd97e67ee github.com/golangci/gofmt v0.0.0-20190930125516-244bba706f1a github.com/golangci/ineffassign v0.0.0-20190609212857-42439a7714cc - github.com/golangci/lint-1 v0.0.0-20190930103755-fad67e08aa89 + github.com/golangci/lint-1 v0.0.0-20191013205115-297bf364a8e0 github.com/golangci/maligned v0.0.0-20180506175553-b1d89398deca github.com/golangci/misspell v0.0.0-20180809174111-950f5d19e770 github.com/golangci/prealloc v0.0.0-20180630174525-215b22d4de21 diff --git a/go.sum b/go.sum index 6b18f1fb..293ced35 100644 --- a/go.sum +++ b/go.sum @@ -94,8 +94,8 @@ github.com/golangci/gofmt v0.0.0-20190930125516-244bba706f1a h1:iR3fYXUjHCR97qWS github.com/golangci/gofmt v0.0.0-20190930125516-244bba706f1a/go.mod h1:9qCChq59u/eW8im404Q2WWTrnBUQKjpNYKMbU4M7EFU= github.com/golangci/ineffassign v0.0.0-20190609212857-42439a7714cc h1:gLLhTLMk2/SutryVJ6D4VZCU3CUqr8YloG7FPIBWFpI= github.com/golangci/ineffassign v0.0.0-20190609212857-42439a7714cc/go.mod h1:e5tpTHCfVze+7EpLEozzMB3eafxo2KT5veNg1k6byQU= -github.com/golangci/lint-1 v0.0.0-20190930103755-fad67e08aa89 h1:664ewjIQUXDvinFMbAsoH2V2Yvaro/X8BoYpIMTWGXI= -github.com/golangci/lint-1 v0.0.0-20190930103755-fad67e08aa89/go.mod h1:66R6K6P6VWk9I95jvqGxkqJxVWGFy9XlDwLwVz1RCFg= +github.com/golangci/lint-1 v0.0.0-20191013205115-297bf364a8e0 h1:MfyDlzVjl1hoaPzPD4Gpb/QgoRfSBR0jdhwGyAWwMSA= +github.com/golangci/lint-1 v0.0.0-20191013205115-297bf364a8e0/go.mod h1:66R6K6P6VWk9I95jvqGxkqJxVWGFy9XlDwLwVz1RCFg= github.com/golangci/maligned v0.0.0-20180506175553-b1d89398deca h1:kNY3/svz5T29MYHubXix4aDDuE3RWHkPvopM/EDv/MA= github.com/golangci/maligned v0.0.0-20180506175553-b1d89398deca/go.mod h1:tvlJhZqDe4LMs4ZHD0oMUlt9G2LWuDGoisJTBzLMV9o= github.com/golangci/misspell v0.0.0-20180809174111-950f5d19e770 h1:EL/O5HGrF7Jaq0yNhBLucz9hTuRzj2LdwGBOaENgxIk= diff --git a/internal/pkgcache/pkgcache.go b/internal/pkgcache/pkgcache.go index d8cdb5ee..86007d04 100644 --- a/internal/pkgcache/pkgcache.go +++ b/internal/pkgcache/pkgcache.go @@ -17,6 +17,14 @@ import ( "github.com/golangci/golangci-lint/pkg/timeutils" ) +type HashMode int + +const ( + HashModeNeedOnlySelf HashMode = iota + HashModeNeedDirectDeps + HashModeNeedAllDeps +) + // Cache is a per-package data cache. A cached data is invalidated when // package or it's dependencies change. type Cache struct { @@ -46,7 +54,7 @@ func (c *Cache) Trim() { }) } -func (c *Cache) Put(pkg *packages.Package, key string, data interface{}) error { +func (c *Cache) Put(pkg *packages.Package, mode HashMode, key string, data interface{}) error { var err error buf := &bytes.Buffer{} c.sw.TrackStage("gob", func() { @@ -59,7 +67,7 @@ func (c *Cache) Put(pkg *packages.Package, key string, data interface{}) error { var aID cache.ActionID c.sw.TrackStage("key build", func() { - aID, err = c.pkgActionID(pkg) + aID, err = c.pkgActionID(pkg, mode) if err == nil { subkey, subkeyErr := cache.Subkey(aID, key) if subkeyErr != nil { @@ -85,11 +93,11 @@ func (c *Cache) Put(pkg *packages.Package, key string, data interface{}) error { var ErrMissing = errors.New("missing data") -func (c *Cache) Get(pkg *packages.Package, key string, data interface{}) error { +func (c *Cache) Get(pkg *packages.Package, mode HashMode, key string, data interface{}) error { var aID cache.ActionID var err error c.sw.TrackStage("key build", func() { - aID, err = c.pkgActionID(pkg) + aID, err = c.pkgActionID(pkg, mode) if err == nil { subkey, subkeyErr := cache.Subkey(aID, key) if subkeyErr != nil { @@ -125,8 +133,8 @@ func (c *Cache) Get(pkg *packages.Package, key string, data interface{}) error { return nil } -func (c *Cache) pkgActionID(pkg *packages.Package) (cache.ActionID, error) { - hash, err := c.packageHash(pkg) +func (c *Cache) pkgActionID(pkg *packages.Package, mode HashMode) (cache.ActionID, error) { + hash, err := c.packageHash(pkg, mode) if err != nil { return cache.ActionID{}, errors.Wrap(err, "failed to get package hash") } @@ -144,12 +152,19 @@ func (c *Cache) pkgActionID(pkg *packages.Package) (cache.ActionID, error) { // packageHash computes a package's hash. The hash is based on all Go // files that make up the package, as well as the hashes of imported // packages. -func (c *Cache) packageHash(pkg *packages.Package) (string, error) { - cachedHash, ok := c.pkgHashes.Load(pkg) +func (c *Cache) packageHash(pkg *packages.Package, mode HashMode) (string, error) { + type hashResults map[HashMode]string + hashResI, ok := c.pkgHashes.Load(pkg) if ok { - return cachedHash.(string), nil + hashRes := hashResI.(hashResults) + if _, ok := hashRes[mode]; !ok { + return "", fmt.Errorf("no mode %d in hash result", mode) + } + return hashRes[mode], nil } + hashRes := hashResults{} + key, err := cache.NewHash("package hash") if err != nil { return "", errors.Wrap(err, "failed to make a hash") @@ -158,13 +173,15 @@ func (c *Cache) packageHash(pkg *packages.Package) (string, error) { fmt.Fprintf(key, "pkgpath %s\n", pkg.PkgPath) for _, f := range pkg.CompiledGoFiles { c.ioSem <- struct{}{} - h, err := cache.FileHash(f) + h, fErr := cache.FileHash(f) <-c.ioSem - if err != nil { - return "", errors.Wrapf(err, "failed to calculate file %s hash", f) + if fErr != nil { + return "", errors.Wrapf(fErr, "failed to calculate file %s hash", f) } fmt.Fprintf(key, "file %s %x\n", f, h) } + curSum := key.Sum() + hashRes[HashModeNeedOnlySelf] = hex.EncodeToString(curSum[:]) imps := make([]*packages.Package, 0, len(pkg.Imports)) for _, imp := range pkg.Imports { @@ -173,20 +190,40 @@ func (c *Cache) packageHash(pkg *packages.Package) (string, error) { sort.Slice(imps, func(i, j int) bool { return imps[i].PkgPath < imps[j].PkgPath }) - for _, dep := range imps { - if dep.PkgPath == "unsafe" { - continue - } - depHash, err := c.packageHash(dep) - if err != nil { - return "", errors.Wrapf(err, "failed to calculate hash for dependency %s", dep.Name) - } + calcDepsHash := func(depMode HashMode) error { + for _, dep := range imps { + if dep.PkgPath == "unsafe" { + continue + } - fmt.Fprintf(key, "import %s %s\n", dep.PkgPath, depHash) + depHash, depErr := c.packageHash(dep, depMode) + if depErr != nil { + return errors.Wrapf(depErr, "failed to calculate hash for dependency %s with mode %d", dep.Name, depMode) + } + + fmt.Fprintf(key, "import %s %s\n", dep.PkgPath, depHash) + } + return nil } - h := key.Sum() - ret := hex.EncodeToString(h[:]) - c.pkgHashes.Store(pkg, ret) - return ret, nil + + if err := calcDepsHash(HashModeNeedOnlySelf); err != nil { + return "", err + } + + curSum = key.Sum() + hashRes[HashModeNeedDirectDeps] = hex.EncodeToString(curSum[:]) + + if err := calcDepsHash(HashModeNeedAllDeps); err != nil { + return "", err + } + curSum = key.Sum() + hashRes[HashModeNeedAllDeps] = hex.EncodeToString(curSum[:]) + + if _, ok := hashRes[mode]; !ok { + return "", fmt.Errorf("invalid mode %d", mode) + } + + c.pkgHashes.Store(pkg, hashRes) + return hashRes[mode], nil } diff --git a/pkg/exitcodes/exitcodes.go b/pkg/exitcodes/exitcodes.go index 7c74a299..536f9036 100644 --- a/pkg/exitcodes/exitcodes.go +++ b/pkg/exitcodes/exitcodes.go @@ -30,3 +30,5 @@ var ( Code: Failure, } ) + +// 1 diff --git a/pkg/golinters/goanalysis/linter.go b/pkg/golinters/goanalysis/linter.go index 5e2d8fcd..bad7ab4f 100644 --- a/pkg/golinters/goanalysis/linter.go +++ b/pkg/golinters/goanalysis/linter.go @@ -11,6 +11,9 @@ import ( "sync/atomic" "time" + "github.com/golangci/golangci-lint/pkg/timeutils" + + "github.com/golangci/golangci-lint/internal/pkgcache" "github.com/golangci/golangci-lint/pkg/logutils" "golang.org/x/tools/go/packages" @@ -332,7 +335,7 @@ func saveIssuesToCache(allPkgs []*packages.Package, pkgsFromCache map[*packages. } atomic.AddInt32(&savedIssuesCount, int32(len(encodedIssues))) - if err := lintCtx.PkgCache.Put(pkg, lintResKey, encodedIssues); err != nil { + if err := lintCtx.PkgCache.Put(pkg, pkgcache.HashModeNeedAllDeps, lintResKey, encodedIssues); err != nil { lintCtx.Log.Infof("Failed to save package %s issues (%d) to cache: %s", pkg, len(pkgIssues), err) } else { issuesCacheDebugf("Saved package %s issues (%d) to cache", pkg, len(pkgIssues)) @@ -379,7 +382,7 @@ func loadIssuesFromCache(pkgs []*packages.Package, lintCtx *linter.Context, defer wg.Done() for pkg := range pkgCh { var pkgIssues []EncodingIssue - err := lintCtx.PkgCache.Get(pkg, lintResKey, &pkgIssues) + err := lintCtx.PkgCache.Get(pkg, pkgcache.HashModeNeedAllDeps, lintResKey, &pkgIssues) cacheRes := pkgToCacheRes[pkg] cacheRes.loadErr = err if err != nil { @@ -430,8 +433,11 @@ func loadIssuesFromCache(pkgs []*packages.Package, lintCtx *linter.Context, } func runAnalyzers(cfg runAnalyzersConfig, lintCtx *linter.Context) ([]result.Issue, error) { - runner := newRunner(cfg.getName(), lintCtx.Log.Child("goanalysis"), - lintCtx.PkgCache, lintCtx.LoadGuard, cfg.getLoadMode()) + log := lintCtx.Log.Child("goanalysis") + sw := timeutils.NewStopwatch("analyzers", log) + defer sw.PrintTopStages(10) + + runner := newRunner(cfg.getName(), log, lintCtx.PkgCache, lintCtx.LoadGuard, cfg.getLoadMode(), sw) pkgs := lintCtx.Packages if cfg.useOriginalPackages() { diff --git a/pkg/golinters/goanalysis/runner.go b/pkg/golinters/goanalysis/runner.go index bd00e77d..5d8470db 100644 --- a/pkg/golinters/goanalysis/runner.go +++ b/pkg/golinters/goanalysis/runner.go @@ -28,6 +28,8 @@ import ( "sync/atomic" "time" + "github.com/golangci/golangci-lint/pkg/timeutils" + "github.com/pkg/errors" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/gcexportdata" @@ -80,9 +82,11 @@ type runner struct { loadMode LoadMode passToPkg map[*analysis.Pass]*packages.Package passToPkgGuard sync.Mutex + sw *timeutils.Stopwatch } -func newRunner(prefix string, logger logutils.Log, pkgCache *pkgcache.Cache, loadGuard *load.Guard, loadMode LoadMode) *runner { +func newRunner(prefix string, logger logutils.Log, pkgCache *pkgcache.Cache, loadGuard *load.Guard, + loadMode LoadMode, sw *timeutils.Stopwatch) *runner { return &runner{ prefix: prefix, log: logger, @@ -90,6 +94,7 @@ func newRunner(prefix string, logger logutils.Log, pkgCache *pkgcache.Cache, loa loadGuard: loadGuard, loadMode: loadMode, passToPkg: map[*analysis.Pass]*packages.Package{}, + sw: sw, } } @@ -481,7 +486,9 @@ func (act *action) analyzeSafe() { act.a.Name, act.pkg.Name, act.isInitialPkg, act.needAnalyzeSource, p), debug.Stack()) } }() - act.analyze() + act.r.sw.TrackStage(act.a.Name, func() { + act.analyze() + }) } func (act *action) analyze() { @@ -803,13 +810,13 @@ func (act *action) persistFactsToCache() error { factsCacheDebugf("Caching %d facts for package %q and analyzer %s", len(facts), act.pkg.Name, act.a.Name) key := fmt.Sprintf("%s/facts", analyzer.Name) - return act.r.pkgCache.Put(act.pkg, key, facts) + return act.r.pkgCache.Put(act.pkg, pkgcache.HashModeNeedAllDeps, key, facts) } func (act *action) loadPersistedFacts() bool { var facts []Fact key := fmt.Sprintf("%s/facts", act.a.Name) - if err := act.r.pkgCache.Get(act.pkg, key, &facts); err != nil { + if err := act.r.pkgCache.Get(act.pkg, pkgcache.HashModeNeedAllDeps, key, &facts); err != nil { if err != pkgcache.ErrMissing { act.r.log.Warnf("Failed to get persisted facts: %s", err) } diff --git a/pkg/golinters/golint.go b/pkg/golinters/golint.go index 1f661b0a..3b1b1b66 100644 --- a/pkg/golinters/golint.go +++ b/pkg/golinters/golint.go @@ -4,6 +4,7 @@ import ( "fmt" "go/ast" "go/token" + "go/types" "sync" lintAPI "github.com/golangci/lint-1" @@ -14,9 +15,10 @@ import ( "github.com/golangci/golangci-lint/pkg/result" ) -func golintProcessPkg(minConfidence float64, files []*ast.File, fset *token.FileSet) ([]result.Issue, error) { +func golintProcessPkg(minConfidence float64, files []*ast.File, fset *token.FileSet, + typesPkg *types.Package, typesInfo *types.Info) ([]result.Issue, error) { l := new(lintAPI.Linter) - ps, err := l.LintASTFiles(files, fset) + ps, err := l.LintPkg(files, fset, typesPkg, typesInfo) if err != nil { return nil, fmt.Errorf("can't lint %d files: %s", len(files), err) } @@ -57,7 +59,7 @@ func NewGolint() *goanalysis.Linter { nil, ).WithContextSetter(func(lintCtx *linter.Context) { analyzer.Run = func(pass *analysis.Pass) (interface{}, error) { - res, err := golintProcessPkg(lintCtx.Settings().Golint.MinConfidence, pass.Files, pass.Fset) + res, err := golintProcessPkg(lintCtx.Settings().Golint.MinConfidence, pass.Files, pass.Fset, pass.Pkg, pass.TypesInfo) if err != nil || len(res) == 0 { return nil, err } @@ -72,5 +74,5 @@ func NewGolint() *goanalysis.Linter { } }).WithIssuesReporter(func(*linter.Context) []goanalysis.Issue { return resIssues - }).WithLoadMode(goanalysis.LoadModeSyntax) + }).WithLoadMode(goanalysis.LoadModeTypesInfo) } diff --git a/pkg/lint/lintersdb/manager.go b/pkg/lint/lintersdb/manager.go index 700552c8..f2cd2bd5 100644 --- a/pkg/lint/lintersdb/manager.go +++ b/pkg/lint/lintersdb/manager.go @@ -76,6 +76,7 @@ func (m Manager) GetAllSupportedLinterConfigs() []*linter.Config { WithPresets(linter.PresetBugs). WithURL("https://github.com/kisielk/errcheck"), linter.NewConfig(golinters.NewGolint()). + WithLoadForGoAnalysis(). WithPresets(linter.PresetStyle). WithURL("https://github.com/golang/lint"), diff --git a/pkg/timeutils/stopwatch.go b/pkg/timeutils/stopwatch.go index 78d1e3cd..9628bd80 100644 --- a/pkg/timeutils/stopwatch.go +++ b/pkg/timeutils/stopwatch.go @@ -10,6 +10,8 @@ import ( "github.com/golangci/golangci-lint/pkg/logutils" ) +const noStagesText = "no stages" + type Stopwatch struct { name string startedAt time.Time @@ -33,11 +35,7 @@ type stageDuration struct { d time.Duration } -func (s *Stopwatch) sprintStages() string { - if len(s.stages) == 0 { - return "no stages" - } - +func (s *Stopwatch) stageDurationsSorted() []stageDuration { stageDurations := []stageDuration{} for n, d := range s.stages { stageDurations = append(stageDurations, stageDuration{ @@ -48,6 +46,16 @@ func (s *Stopwatch) sprintStages() string { sort.Slice(stageDurations, func(i, j int) bool { return stageDurations[i].d > stageDurations[j].d }) + return stageDurations +} + +func (s *Stopwatch) sprintStages() string { + if len(s.stages) == 0 { + return noStagesText + } + + stageDurations := s.stageDurationsSorted() + stagesStrings := []string{} for _, s := range stageDurations { stagesStrings = append(stagesStrings, fmt.Sprintf("%s: %s", s.name, s.d)) @@ -56,6 +64,22 @@ func (s *Stopwatch) sprintStages() string { return fmt.Sprintf("stages: %s", strings.Join(stagesStrings, ", ")) } +func (s *Stopwatch) sprintTopStages(n int) string { + if len(s.stages) == 0 { + return noStagesText + } + + stageDurations := s.stageDurationsSorted() + + stagesStrings := []string{} + for i := 0; i < len(stageDurations) && i < n; i++ { + s := stageDurations[i] + stagesStrings = append(stagesStrings, fmt.Sprintf("%s: %s", s.name, s.d)) + } + + return fmt.Sprintf("top %d stages: %s", n, strings.Join(stagesStrings, ", ")) +} + func (s *Stopwatch) Print() { p := fmt.Sprintf("%s took %s", s.name, time.Since(s.startedAt)) if len(s.stages) == 0 { @@ -74,6 +98,14 @@ func (s *Stopwatch) PrintStages() { s.log.Infof("%s took %s with %s", s.name, stagesDuration, s.sprintStages()) } +func (s *Stopwatch) PrintTopStages(n int) { + var stagesDuration time.Duration + for _, s := range s.stages { + stagesDuration += s + } + s.log.Infof("%s took %s with %s", s.name, stagesDuration, s.sprintTopStages(n)) +} + func (s *Stopwatch) TrackStage(name string, f func()) { startedAt := time.Now() f() diff --git a/vendor/github.com/golangci/lint-1/lint.go b/vendor/github.com/golangci/lint-1/lint.go index de63631a..886c85bf 100644 --- a/vendor/github.com/golangci/lint-1/lint.go +++ b/vendor/github.com/golangci/lint-1/lint.go @@ -118,10 +118,12 @@ func (l *Linter) LintFiles(files map[string][]byte) ([]Problem, error) { // LintFiles lints a set of files of a single package. // The argument is a map of filename to source. -func (l *Linter) LintASTFiles(files []*ast.File, fset *token.FileSet) ([]Problem, error) { +func (l *Linter) LintPkg(files []*ast.File, fset *token.FileSet, typesPkg *types.Package, typesInfo *types.Info) ([]Problem, error) { pkg := &pkg{ - fset: fset, - files: make(map[string]*file), + fset: fset, + files: make(map[string]*file), + typesPkg: typesPkg, + typesInfo: typesInfo, } var pkgName string for _, f := range files { @@ -193,25 +195,6 @@ type pkg struct { } func (p *pkg) lint() []Problem { - if err := p.typeCheck(); err != nil { - /* TODO(dsymonds): Consider reporting these errors when golint operates on entire packages. - if e, ok := err.(types.Error); ok { - pos := p.fset.Position(e.Pos) - conf := 1.0 - if strings.Contains(e.Msg, "can't find import: ") { - // Golint is probably being run in a context that doesn't support - // typechecking (e.g. package files aren't found), so don't warn about it. - conf = 0 - } - if conf > 0 { - p.errorfAt(pos, conf, category("typechecking"), e.Msg) - } - - // TODO(dsymonds): Abort if !e.Soft? - } - */ - } - p.scanSortable() p.main = p.isMain() diff --git a/vendor/modules.txt b/vendor/modules.txt index d70dc6fd..9ed88716 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -72,7 +72,7 @@ github.com/golangci/gofmt/gofmt github.com/golangci/gofmt/goimports # github.com/golangci/ineffassign v0.0.0-20190609212857-42439a7714cc github.com/golangci/ineffassign -# github.com/golangci/lint-1 v0.0.0-20190930103755-fad67e08aa89 +# github.com/golangci/lint-1 v0.0.0-20191013205115-297bf364a8e0 github.com/golangci/lint-1 # github.com/golangci/maligned v0.0.0-20180506175553-b1d89398deca github.com/golangci/maligned