Speed up loading of source code #323

Helcaraxan · 2020-08-05T16:23:48Z

The current approach of loading the source code of targeted packages has some inefficiencies that may induce considerable slowdowns and unnecessary lengthy runs when using mockery on larger codebases. Most notably it always defaults to loading individual files, even when specifying an entire package as target.

This PR speeds up the processing by rewiring the Parse and Walk methods to accept generic target patterns instead of only directory and file paths. This removes the need for the CLI tool to invoke package.Load() to determine the underlying files when given a package pattern and also means that all the source code is loaded in a single call to package.Load() instead of one call per source file in the targeted package(s).

A secondary advantage of this rewiring is that the Walker struct and it's API now also accept a package pattern in its BaseDir field making it more user friendly for users invoking the internals of mockery themselves instead of using the command-line tool as they benefit from the same speed-up (I am one of those).

There are no breaking changes in either the API or the semantics of the CLI tool and its implementation packages.

The PR also contains a few drive-by fixes:

Walker.doWalk now properly reports errors.
The removal of an unused test utility function.
The fixing of tests that were silently failing due to Walker.doWalk not properly reporting errors.
The deprecation of the Walker.LimitOne field as it has no longer any speed-up benefit when set internally by the CLI side.

Some anecdotal testing on a large private codebase that generates 100s of mocks from ~150 packages by calling Walker.Walk via internal tooling shows a runtime reduction of 65+% from 180 seconds down to 50-60.

LandonTClipp · 2020-08-05T18:06:05Z

Fantastic work, I will review this more in-depth over the coming days when I get some free time. Let's get this merged.

pkg/parse.go

LandonTClipp · 2020-08-06T00:53:36Z

pkg/parse.go

+		// A file should be passed directly to the package loader as a 'file' query.
+		if filepath.Ext(pattern) != ".go" {
+			return fmt.Errorf("specified file %q cannot be parsed as it is not a source file", pattern)
+		} else if strings.HasPrefix(info.Name(), ".") || strings.HasPrefix(info.Name(), "_") {


Is this not different behavior? I may be wrong but I dont' see anywhere where mockery would have skipped .* or _* files.

This is some existing logic that I took from Walker and moved here. I am not partial to keeping or removing it, I just moved to keep the existing semantics. 🙂

Technically it would be a breaking change to remove this.

pkg/parse.go

LandonTClipp

Looks good, but made a few requests for change.

Helcaraxan

Thank you for the quick review. I addressed / responded to your comments. 🙂

As a side-note, and not something for this specific PR: with the changes I made there's no real use-case any more as far as I'm seeing for allowing people to specify targets as directories in addition to package patterns. Package patterns are strictly more expressive than directories (in the constraint of referencing Go source code), without missing out on any edge-cases that are not unsafe by default.

To explicit that last statement: one could showcase the situation of two Go modules in the same repository, one at the root (A) and one inside it (A/B) and the desire when running at the root to generate a mock in A/mock within module A, for an interface in A/B. It would simply be a case of invoking mockery -dir=./B [...]. However this would be allowing someone to shoot themselves in the foot as the source code in that directory is not necessarily the one used by A at all. Instead A might depend on an entirely different version of A/B with different interfaces instead of using a relative replace statement in it's go.mod.

It's IMO better to require people to only specify package patterns (which may still use relative references inside the same module like ./C/... from the root of A for A/C/...) as it will guarantee that Go's native dependency resolution is respected.

Secondary point: it would considerably simplify the logic that I just rewired. 😉

This would obviously be a breaking change from both an API perspective as well as the CLI point-of view so would need to wait for a v3.

Helcaraxan · 2020-08-06T08:17:34Z

pkg/parse.go

+		// A file should be passed directly to the package loader as a 'file' query.
+		if filepath.Ext(pattern) != ".go" {
+			return fmt.Errorf("specified file %q cannot be parsed as it is not a source file", pattern)
+		} else if strings.HasPrefix(info.Name(), ".") || strings.HasPrefix(info.Name(), "_") {


This is some existing logic that I took from Walker and moved here. I am not partial to keeping or removing it, I just moved to keep the existing semantics. 🙂

Technically it would be a breaking change to remove this.

Helcaraxan · 2020-08-10T09:15:14Z

Quick and friendly ping on this @LandonTClipp. No worries if you are too busy as I very much understand that other things may take priority. Just did not want it to slip of your radar. 🙂

LandonTClipp · 2020-08-10T19:17:23Z

Thanks for the ping, I will not let it slip! Life got in the way (car transmission got totaled, big fun). Also extra busy this week but I will be sure to come back.

LandonTClipp · 2020-08-10T20:13:52Z

There have been other places where people suggested moving towards a package-specify model instead of file-specify model, and I'm in total agreement. It's much better to leverage the code already given in package.Load. You are right, however, that it would be a breaking change, which is why we need to wait for v3.

https://github.com/vektra/mockery/projects/3

I'm still in the process of collecting requirements for v3, and also want to finish/incorporate my other project into v3 https://github.com/chigopher/pathlib.

LandonTClipp · 2020-08-13T03:08:52Z

Unfortunately it looks like this has a major bug in it:

12 Aug 20 22:07 CDT DBG Loading files in directory ".git/objects/0f". dry-run=false version=0.0.0-dev
12 Aug 20 22:07 CDT DBG parsing dry-run=false version=0.0.0-dev
^C
[ltclipp@landon-virtualbox pathlib]$

It wants to iterate over dot-dirs, so we have a regression.

LandonTClipp reviewed Aug 6, 2020

View reviewed changes

pkg/parse.go Outdated Show resolved Hide resolved

LandonTClipp reviewed Aug 6, 2020

View reviewed changes

pkg/parse.go Outdated Show resolved Hide resolved

LandonTClipp reviewed Aug 6, 2020

View reviewed changes

pkg/parse.go Outdated Show resolved Hide resolved

LandonTClipp requested changes Aug 6, 2020

View reviewed changes

Speed up loading of source code

a581abb

Helcaraxan force-pushed the load-speedup branch from d49aee6 to a581abb Compare August 6, 2020 08:22

Helcaraxan commented Aug 6, 2020

View reviewed changes

LandonTClipp approved these changes Aug 13, 2020

View reviewed changes

LandonTClipp merged commit 6d9ebaa into vektra:master Aug 13, 2020

This was referenced Aug 13, 2020

Revert "Speed up loading of source code" #326

Merged

is there any tips to speedup mockery generation? #328

Closed

Helcaraxan mentioned this pull request Aug 17, 2020

Speed up loading of source code - v2 #329

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up loading of source code #323

Speed up loading of source code #323

Helcaraxan commented Aug 5, 2020 •

edited

Loading

LandonTClipp commented Aug 5, 2020

LandonTClipp Aug 6, 2020

Helcaraxan Aug 6, 2020

LandonTClipp left a comment

Helcaraxan left a comment •

edited

Loading

Helcaraxan Aug 6, 2020

Helcaraxan commented Aug 10, 2020

LandonTClipp commented Aug 10, 2020

LandonTClipp commented Aug 10, 2020

LandonTClipp commented Aug 13, 2020 •

edited

Loading

Speed up loading of source code #323

Speed up loading of source code #323

Conversation

Helcaraxan commented Aug 5, 2020 • edited Loading

LandonTClipp commented Aug 5, 2020

LandonTClipp Aug 6, 2020

Choose a reason for hiding this comment

Helcaraxan Aug 6, 2020

Choose a reason for hiding this comment

LandonTClipp left a comment

Choose a reason for hiding this comment

Helcaraxan left a comment • edited Loading

Choose a reason for hiding this comment

Helcaraxan Aug 6, 2020

Choose a reason for hiding this comment

Helcaraxan commented Aug 10, 2020

LandonTClipp commented Aug 10, 2020

LandonTClipp commented Aug 10, 2020

LandonTClipp commented Aug 13, 2020 • edited Loading

Helcaraxan commented Aug 5, 2020 •

edited

Loading

Helcaraxan left a comment •

edited

Loading

LandonTClipp commented Aug 13, 2020 •

edited

Loading