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

Do not load unnecessary package information #203

Merged
merged 2 commits into from
Aug 13, 2023

Conversation

samherrmann
Copy link
Contributor

@samherrmann samherrmann commented Aug 13, 2023

This merge request is a performance improvement to address #178. The merge request contains two commits, in which the first commit adds a benchmark test. This allows the benchmark test to be executed before and after the changes in the second commit for comparison. Further optimization may be required to close #178, but I hope that this merge request is a step in the right direction.

The following are the benchmark results on my dev machine:

Before:

$ go test -bench=. ./internal/registry -count 3
goos: linux
goarch: amd64
pkg: github.com/matryer/moq/internal/registry
cpu: Intel(R) Core(TM) i7-4790 CPU @ 3.60GHz
BenchmarkNew-8                 3         335638233 ns/op
BenchmarkNew-8                 4         328678100 ns/op
BenchmarkNew-8                 4         322492350 ns/op
PASS
ok      github.com/matryer/moq/internal/registry        8.735s

After:

go test -bench=. ./internal/registry -count 3
goos: linux
goarch: amd64
pkg: github.com/matryer/moq/internal/registry
cpu: Intel(R) Core(TM) i7-4790 CPU @ 3.60GHz
BenchmarkNew-8                15          76012420 ns/op
BenchmarkNew-8                14          76392171 ns/op
BenchmarkNew-8                14          76113471 ns/op
PASS
ok      github.com/matryer/moq/internal/registry        3.520s

The following shows an approximate time improvement to execute all tests on my dev machine:

Before:

go clean -testcache && time go test ./...
?       github.com/matryer/moq  [no test files]
?       github.com/matryer/moq/generate [no test files]
ok      github.com/matryer/moq/example  0.002s [no tests to run]
ok      github.com/matryer/moq/internal/registry        0.002s [no tests to run]
ok      github.com/matryer/moq/internal/template        0.002s
ok      github.com/matryer/moq/pkg/moq  11.691s

real    0m17.944s
user    0m28.584s
sys     0m3.847s

After:

$ go clean -testcache && time go test ./...
?       github.com/matryer/moq  [no test files]
?       github.com/matryer/moq/generate [no test files]
ok      github.com/matryer/moq/example  0.002s [no tests to run]
ok      github.com/matryer/moq/internal/registry        0.002s [no tests to run]
ok      github.com/matryer/moq/internal/template        0.002s
ok      github.com/matryer/moq/pkg/moq  3.704s

real    0m9.468s
user    0m4.982s
sys     0m2.330s

This commit is in response to issue 178, but it may not be enough to
close the issue.
@@ -173,14 +176,14 @@ func pkgInfoFromPath(srcDir string, mode packages.LoadMode) (*packages.Package,
return pkgs[0], nil
}

func findPkgPath(pkgInputVal string, srcPkg *packages.Package) string {
func findPkgPath(pkgInputVal string, srcPkgPath string) string {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the argument so that we don't pass more into the function than what's needed.

@@ -195,9 +198,9 @@ func pkgInDir(pkgName, dir string) bool {
return currentPkg.Name == pkgName || currentPkg.Name+"_test" == pkgName
}

func parseImportsAliases(pkg *packages.Package) map[string]string {
func parseImportsAliases(syntaxTree []*ast.File) map[string]string {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the argument so that we don't pass more into the function than what's needed.

}

// New loads the source package info and returns a new instance of
// Registry.
func New(srcDir, moqPkg string) (*Registry, error) {
srcPkg, err := pkgInfoFromPath(
srcDir, packages.NeedName|packages.NeedSyntax|packages.NeedTypes|packages.NeedTypesInfo|packages.NeedDeps,
srcDir, packages.NeedName|packages.NeedSyntax|packages.NeedTypes,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the docs:

// NeedTypesInfo adds TypesInfo.
NeedTypesInfo

// NeedDeps adds the fields requested by the LoadMode in the packages in Imports.
NeedDeps

Neither TypesInfo or Imports field is used, therefore there is no need to load this information.

srcPkgTypes: srcPkg.Types,
moqPkgPath: findPkgPath(moqPkg, srcPkg.PkgPath),
aliases: parseImportsAliases(srcPkg.Syntax),
imports: make(map[string]*Package),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

srcPkg only has the fields populated that were instructed to be populated by the packages.LoadMode (line 31). It is therefore not ideal to let srcPkg outside of this New function because it opens up the possibility for other areas of the code to depend on fields that were not loaded. It is also more difficult to see if other areas of the code actually require certain packages.LoadMode flags. By not passing srcPkg outside of this New function, it is now easier to see which packages.LoadMode flags are truly needed.

@samherrmann samherrmann marked this pull request as ready for review August 13, 2023 03:15
@sudo-suhas sudo-suhas merged commit c59089b into matryer:main Aug 13, 2023
4 checks passed
@sudo-suhas
Copy link
Collaborator

Thanks for the perf improvement change @samherrmann 🎉

@samherrmann samherrmann deleted the issue-178 branch August 13, 2023 11:37
@hypnoglow
Copy link

I can confirm that for one of our projects the generation time reduced significantly.

Before:

task gen  131.44s user 31.98s system 611% cpu 26.742 total

After:

task gen  17.54s user 24.17s system 485% cpu 8.582 total

@fsaintjacques
Copy link

Is it possible to tag this? We install moq via go.mod, it would be nice if we could pin it to a specific stable version.

@sudo-suhas
Copy link
Collaborator

I have added a tag - https://github.com/matryer/moq/releases/tag/v0.3.3. However, there was a problem with goreleaser config and it did not attach the binaries to the release. Will fix the goreleaser config when I can, until then, hope the tag suffices.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is there a way to speed up the moq generation time?
4 participants