Skip to content

Regressions related to 4242f24 #995

@ldez

Description

@ldez

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).

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions