Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/tools/go/packages: bad performance #30677

Closed
anjmao opened this issue Mar 8, 2019 · 10 comments
Closed

x/tools/go/packages: bad performance #30677

anjmao opened this issue Mar 8, 2019 · 10 comments

Comments

@anjmao
Copy link

anjmao commented Mar 8, 2019

Problem

There is quite a lot of issues regarding go/packages performance. All tools which a migrating to go modules a going to use x/tools/go/packages which is undesirable slow.

go version go1.11.2 darwin/amd64

Steps to reproduce

parse_me.go

package dotest

import (
	"fmt"
	"time"
)

func bar() {
	fmt.Println(time.Now())
}

benchmark_test.go

package main

import (
	"fmt"
	"go/parser"
	"go/token"
	"log"
	"os"
	"testing"

	"golang.org/x/tools/go/packages"
)

const (
	rootPath = "<path_to_folder>"
	filePath = "<path_to_folder>/parse_me.go"
)

func BenchmarkParseWithStd(b *testing.B) {
	for i := 0; i < b.N; i++ {
		parseWithStd()
	}
}

func BenchmarkLoadAllSyntax(b *testing.B) { benchmarkLoadPkg(b, packages.LoadAllSyntax) }

func BenchmarkLoadSyntax(b *testing.B) { benchmarkLoadPkg(b, packages.LoadSyntax) }

func BenchmarkLoadTypes(b *testing.B) { benchmarkLoadPkg(b, packages.LoadTypes) }

func BenchmarkLoadImports(b *testing.B) { benchmarkLoadPkg(b, packages.LoadImports) }

func benchmarkLoadPkg(b *testing.B, loadMode packages.LoadMode) {
	for i := 0; i < b.N; i++ {
		parseWithModules(loadMode)
	}
}

func parseWithStd() {
	fset := token.NewFileSet()
	_, err := parser.ParseFile(fset, filePath, nil, parser.AllErrors|parser.ParseComments)
	if err != nil {
		log.Fatal(err)
	}
}

func parseWithModules(loadMode packages.LoadMode) {
	fset := token.NewFileSet()
	cfg := &packages.Config{
		Dir:     rootPath,
		Mode:    loadMode,
		Fset:    fset,
		Overlay: make(map[string][]byte),
		Tests:   true,
	}
	pkgs, err := packages.Load(cfg, fmt.Sprintf("file=%s", filePath))
	if err != nil {
		log.Fatal(err)
	}
	if packages.PrintErrors(pkgs) > 0 {
		os.Exit(1)
	}
}

Results

BenchmarkParseWithStd-8    	   50000	     33231 ns/op	    3632 B/op	      68 allocs/op
BenchmarkLoadAllSyntax-8   	       2	 717116925 ns/op	138813840 B/op	 1329883 allocs/op
BenchmarkLoadSyntax-8      	      10	 132804220 ns/op	 2685484 B/op	    5810 allocs/op
BenchmarkLoadTypes-8       	      10	 131061186 ns/op	  435418 B/op	    3812 allocs/op
BenchmarkLoadImports-8     	      10	 112006365 ns/op	  390102 B/op	    3423 allocs/op

Is somebody working on it? If not I'm happy to help.

Related issues:
#29758
#29452
#29427

@gopherbot gopherbot added this to the Unreleased milestone Mar 8, 2019
@agnivade
Copy link
Contributor

agnivade commented Mar 9, 2019

This may be too broad of an issue. I believe there are more specific issues already filed which tracks specific areas where performance can be improved.

/cc @ianthehat @matloob

@anjmao
Copy link
Author

anjmao commented Mar 9, 2019

I linked few issues which I found. I found only #29427 which is more specific as it uses cgo in the import path, but for my case it is slow in even for hello world example.

@mvdan
Copy link
Member

mvdan commented Mar 9, 2019

You should also clarify what Go version you're on. For example, #29382 is making the go binary initialize (start) faster on 1.13, so you should get better numbers on master.

@anjmao
Copy link
Author

anjmao commented Mar 9, 2019

@mvdan Thanks, I added go version which I'm using. I will try master version and update benchmarks.

@mvdan
Copy link
Member

mvdan commented Mar 9, 2019

The benchmark is also not too fair; you're comparing parsing a single file with loading an entire package. The "before" benchmark should be using something like https://godoc.org/golang.org/x/tools/go/loader to be more realistic, as that too loaded Go packages.

I realise your purpose is still to parse a single file on disk. But if that's all you need, I don't think you're ever going to get performance anywhere close to a single go/parser.ParseFile call. If all you need is parsing a single file, that's what you should be using.

In particular, modes like LoadSyntax also typecheck the loaded package, so that's definitely more work than just parsing a single file (or a single package).

@mvdan
Copy link
Member

mvdan commented Mar 9, 2019

Another bit of unfairness I noticed - you're using Tests: true, so you're likely loading the package twice. Search for fmt.test in https://godoc.org/golang.org/x/tools/go/packages for some examples of why that's the case. If the "before" benchmarks aren't loading test packages, the new ones shouldn't either.

@mvdan
Copy link
Member

mvdan commented Mar 9, 2019

I also forgot about the elephant in the room; your "before" benchmark, even if it uses x/tools/go/loader, does not support loading packages in module mode. That's a whole added level of complexity that go/packages has to deal with.

@anjmao
Copy link
Author

anjmao commented Mar 9, 2019

  1. I tried such example there you don't have any imports and got the same results.
package main
func main() {
}
  1. Tests: true doesn't impact bench results for my example.
  2. I understand that go/packages must do more work, but if it is that slow for "empty" file there you have no imports something is definitely not going well.

@mvdan
Copy link
Member

mvdan commented Mar 9, 2019

I'm still not sure I understand what you're trying to do. If you just need to parse a file, you should stick to ParseFile. If you need to load entire packages with support for Go modules, use go/packages.

There are probably areas for optimizations and speed-ups in go/packages, but you're still comparing apples and oranges with the ParseFile micro-benchmark.

@anjmao
Copy link
Author

anjmao commented Mar 10, 2019

@mvdan Thanks, you are right. For parsing a file I will stick to ParseFile and cache go/packages result. I'm closing this issue as there are more specific issues due go/packages performances.

@anjmao anjmao closed this as completed Mar 10, 2019
@golang golang locked and limited conversation to collaborators Mar 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants