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.

Sign up for GitHub

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

Jump to bottom

Regressions related to 4242f24 #995

Closed
ldez opened this issue Jun 3, 2024 · 5 comments · Fixed by #998
Closed

Regressions related to 4242f24 #995

ldez opened this issue Jun 3, 2024 · 5 comments · Fixed by #998

Comments

@ldez
Copy link
Contributor

ldez commented Jun 3, 2024 •
edited
Loading

PR #993 creates a performance regression because go list is expensive and will be called on each package (even when range-val-address is not used).

"Funny thing", the regression impacts all the rules except range-val-address because this rule is skipped with go1.22.

It will be a performance regression for revive and for golangci-lint.
For golangci-lint, a way to define the Go version externally to bypass the go list is a solution.
I don't know enough about the revive design to propose a solution for revive.

We encountered the same problem with gosec golangci/golangci-lint#4735

log the call to detectGoVersion on revive's code 
$ cd revive
$ ./revive ./...
detectGoVersion
detectGoVersion
detectGoVersion
detectGoVersion
detectGoVersion
detectGoVersion
detectGoVersion
detectGoVersion
detectGoVersion
detectGoVersion
detectGoVersion
rule/datarace.go:83:3: var getIds should be getIDs
lint/file.go:191:22: parameter 'filename' seems to be unused, consider removing or renaming it as _
benchmark on kubernetes 
# sample.toml
ignoreGeneratedHeader = false
severity = "warning"
confidence = 0.8
errorCode = 0
warningCode = 0

#[rule.range-val-address]
[rule.context-as-argument]
$ cd kubernetes
$ rm go.work go.work.sum
$ hyperfine './revive-bbe5eb7  -config sample.toml ./...' './revive-4242f24  -config sample.toml ./...'
Benchmark 1: ./revive-bbe5eb7  -config sample.toml ./...
  Time (mean ± σ):      1.989 s ±  0.125 s    [User: 11.202 s, System: 1.519 s]
  Range (min … max):    1.796 s …  2.220 s    10 runs
 
Benchmark 2: ./revive-4242f24  -config sample.toml ./...
  Time (mean ± σ):      3.880 s ±  0.171 s    [User: 22.235 s, System: 14.092 s]
  Range (min … max):    3.633 s …  4.193 s    10 runs
 
Summary
  ./revive-bbe5eb7  -config sample.toml ./... ran
    1.95 ± 0.15 times faster than ./revive-4242f24  -config sample.toml ./...
$ cd revive
$ git lg
* 4242f24 - Add support for the new implementation of for loop variables in go 1.22. (#993)
* bbe5eb7 - fix(deps): update module github.com/burntsushi/toml to v1.4.0 (#992)
...

With the commit, the run of context-as-argument (just an example) is about 2x slower than the previous commit (and about 10x slower on the system).

Also, the implementation has a bug with Go workspaces:

can't parse the output of go list: invalid character '{' after top-level value

Inside a Go workspace, go list always returns all the modules, not just the current module.

As a reference, a working implementation: https://github.com/golangci/modinfo/blob/main/module.go

$ cd kubernetes
$ go list -m -json                                
{
        "Path": "k8s.io/kubernetes",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/api",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/api",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/api/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/apiextensions-apiserver",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/apiextensions-apiserver",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/apiextensions-apiserver/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/apimachinery",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/apimachinery",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/apimachinery/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/apiserver",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/apiserver",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/apiserver/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/cli-runtime",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/cli-runtime",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/cli-runtime/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/client-go",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/client-go",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/client-go/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/cloud-provider",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/cloud-provider",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/cloud-provider/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/cluster-bootstrap",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/cluster-bootstrap",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/cluster-bootstrap/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/code-generator",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/code-generator",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/code-generator/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/component-base",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/component-base",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/component-base/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/component-helpers",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/component-helpers",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/component-helpers/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/controller-manager",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/controller-manager",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/controller-manager/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/cri-api",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/cri-api",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/cri-api/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/cri-client",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/cri-client",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/cri-client/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/csi-translation-lib",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/csi-translation-lib",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/csi-translation-lib/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/dynamic-resource-allocation",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/dynamic-resource-allocation",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/dynamic-resource-allocation/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/endpointslice",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/endpointslice",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/endpointslice/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/kms",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kms",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kms/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/kube-aggregator",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kube-aggregator",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kube-aggregator/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/kube-controller-manager",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kube-controller-manager",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kube-controller-manager/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/kube-proxy",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kube-proxy",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kube-proxy/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/kube-scheduler",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kube-scheduler",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kube-scheduler/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/kubectl",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kubectl",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kubectl/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/kubelet",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kubelet",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/kubelet/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/metrics",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/metrics",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/metrics/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/mount-utils",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/mount-utils",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/mount-utils/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/pod-security-admission",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/pod-security-admission",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/pod-security-admission/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/sample-apiserver",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/sample-apiserver",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/sample-apiserver/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/sample-cli-plugin",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/sample-cli-plugin",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/sample-cli-plugin/go.mod",
        "GoVersion": "1.22.0"
}
{
        "Path": "k8s.io/sample-controller",
        "Main": true,
        "Dir": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/sample-controller",
        "GoMod": "/home/ldez/sources/experimental/kubernetes/staging/src/k8s.io/sample-controller/go.mod",
        "GoVersion": "1.22.0"
}

FYI, the issue comment (golang/go#44753 (comment)), used as a reference inside the PR, is outdated since Go workspace (go1.18).
@ldez ldez changed the title Performance regression related to 4242f24 Regressions related to 4242f24 Jun 3, 2024
@ldez ldez mentioned this issue Jun 3, 2024
Merged
@denisvmedia
Copy link
Collaborator

@ldez thanks for your report!

@chavacava should we revert it?

cc @dominiquelefevre

@dominiquelefevre
Copy link
Contributor

#998

@ldez
Copy link
Contributor Author

ldez commented Jun 3, 2024 •
edited
Loading

From the golangci-lint POV, we need an option to set the Go version instead of detecting it inside revive.

@ldez ldez mentioned this issue Jun 3, 2024
Merged
@dominiquelefevre
Copy link
Contributor

Ping @denisvmedia, @chavacava, @ldez.

@dominiquelefevre
Copy link
Contributor

Hey, is anybody here to read the PR that fixes this issue? @denisvmedia @chavacava @ldez ?

chavacava pushed a commit that referenced this issue Jun 22, 2024
@dominiquelefevre
Fix a performance regression #995 (#998)
0df1bb0 
* Support go workspaces when detecting the go version.

When a module is part of a workspace, a call to `go list -m` lists all
modules in the workspace, and we need to parse multiple modinfos.

* Do not invoke `go list` for every package.

* Add a go language version override config option for golangci-lint.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix a performance regression #995 dominiquelefevre/revive
3 participants
@denisvmedia @ldez @dominiquelefevre