gcexportdata.NewImporter: huge impact on performance

[ldez]

Describe the bug

gcexportdata.Find has been rewritten:

• https://go-review.googlesource.com/c/tools/+/310515
• golang/tools@2a90056

gcexportdata.NewImporter uses gcexportdata.Find.

And revive uses gcexportdata.NewImporter:

revive/lint/package.go

Line 30 in 60e1ae4

return gcexportdata.NewImporter(fset, make(map[string]*types.Package))

To Reproduce
Steps to reproduce the behavior:

1. update golang.org/x/tools to v0.1.12-0.20220628192153-7743d1d949f1
2. build a binary
3. clone https://github.com/kubernetes-sigs/cluster-api
4. run the binary

Expected behavior
Run fast

Logs

Desktop (please complete the following information):

• OS: Linux
• Version of Go: all versions

Additional context

gcexportdata.NewImporter and gcexportdata.Find are now deprecated.

golangci/golangci-lint#2997 (comment)

[ldez]

@chavacava @mgechev this has a really huge impact on performance.
I think you will be faster than me to fix that because I don't know the core design of revive, but if you need help I will be happy to contribute.

[trim21]

it looks like this function is only used for go files' type checking.

I'm not very formally with these go build tools, don't know what's the best replacement for it.

[ldez]

@trim21 The replacement is defined inside the deprecation notice: https://github.com/golang/tools/blob/79f3242e4b2ee6f1bd987fdd0538e16451f7523e/go/gcexportdata/importer.go#L26

[chavacava]

Hi @ldez, thanks for proposing this improvement. I'll work on it in the next days.

[chavacava]

Update: I have a working version where the problematic lib was replaced with go/packages (huge modif)
PR: #716

TODO:

1. check if perf is better wrt the master branch
2. fix test data (go/packages requires go files to be compilable... testdata are syntactically OK but it do not necessarily compile :( )
3. only type check if a type-dependent rule is activated (at the moment type information is always calculated) This has an impact on performance.
4. clean-up the code

[chavacava]

Using packages in place of gcexportdata beaks some (important) things and I did not find simple solutions:

• Now the code to be analyzed needs to compile. Before a syntactically correct code was enough. (will need to update all the testdata)
• Now revive needs to be aware if the analyzed code is developed as a module or not. Before revive just run on any code base

[ldez]

golang.org/x/tools has created a release v0.12.0 with the new implementation of gcexportdata.Find.

I don't know how I can help you.

related to #720

[mgechev]

I'll connect with the Go team and see if there's anything we can do.

[mgechev]

Looks like lots of folks are out of office. Seems like NewImporter is deprecated. I put together a quick fix. Wondering if it resolves the performance issue? #723

Could you check and let me know?

[ldez]

you can clone https://github.com/kubernetes-sigs/cluster-api to test your PR

[ldez]

I tested your PR, and it seems to work, but I still need to test it inside golangci-lint.

[trim21]

tested, looks good

[ldez]

I did tests with golangci-lint and it seems to work as expected.

[mgechev]

Thanks for giving it a try! I'l let @chavacava review and merge.

[mgechev]

Released in v1.2.2.

[mgechev]

The release failed. @doniacld, @Clivern if you have a moment would you check .goreleaser? Looks like it complains about failing build for windows arm64.

[mgechev]

Moved windows_arm64 to the ignore list of .goreleaser.yml for now.

[ldez]

Maybe you can create a another issue to track this problem?

[Clivern]

it seems goreleaser dropped windows arm64 support for go < 1.17 for some reasons or a new feature.

$ goreleaser check

   • loading config file       file=.goreleaser.yml
   • checking config:
   • DEPRECATED: skipped windows/arm64 build on Go < 1.17 for compatibility, check https://goreleaser.com/deprecations/#builds-for-windowsarm64 for more info.
   • config is valid

Anyways i opened a PR to use 1.19 for release since i see we did the upgrade! also removed 1.16 build that keeps failing. #727

[mgechev]

Good catch, @Clivern! Thank you for the fix.