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

Use x/tools/go/packages #147

Closed
vincepri opened this issue Jun 25, 2019 · 14 comments
Closed

Use x/tools/go/packages #147

vincepri opened this issue Jun 25, 2019 · 14 comments
Assignees
@palnabarun @nikhita
Labels
lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.

Comments

@vincepri
Copy link
Member

vincepri commented Jun 25, 2019 •
edited
Loading

To better support modules, we should plan to switch to the x/tools/go/package library to scan Go packages.

This issue is a follow up from kubernetes/code-generator#69.
@nikhita nikhita mentioned this issue Jul 16, 2019
Closed
@joelanford joelanford mentioned this issue Aug 23, 2019
Closed
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 23, 2019
@nikhita
Copy link
Member

nikhita commented Sep 23, 2019 via email

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 23, 2019
@charith-elastic charith-elastic mentioned this issue Oct 28, 2019
Merged
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 22, 2019
@vincepri
Copy link
Member Author

/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 22, 2019
@tbe
Copy link

tbe commented Apr 14, 2020

I would like to implement this.

As i see it for now, this would replace most of

  • addDir
  • importBuildPackage
  • importPackage
  • findTypesIn
  • FindTypes

As we could get the types.Package object for each package during the initial packages.Load(), the FindTypes procedure would result very much in a stub (for compatibility) and the universe could be filled during the import phase.

Anything i got way off or missed?

@lavalamp
Copy link
Member

I'd be happy to see this modernized. I don't have the cycles to help much, though.

@tbe
Copy link

tbe commented Apr 14, 2020 via email

@tbe
Copy link

tbe commented Apr 15, 2020

This may have to wait until golang/go#38470 is solved.

@invidian
Copy link
Member

kubernetes/code-generator#69 is really annoying.

@tbe as golang/go#38470 doesn't seem to see much progress, how about we rename testdata package to internal? Would it unblock your work?

I did that for testing and all tests passes, so I assume it should be fine? For go run test, I had to filter out k8s.io/gengo/internal, as go list lists those packages. Calling golang.org/x/tools/go/packages.Load(nil, "k8s.io/gengo/internal/a/...") also seems fine.

@invidian
Copy link
Member

Okay, I went waaay to deep with this 😂

Sooo, when running code generation on projects like https://github.com/kubernetes-sigs/cluster-api project, generation takes a lot of time.

With ps I noticed that this process spawns a lot of go list ... commands.

Running with perf or pkg/profile confirms, that actual Go execution time is only a fraction of total execution time.

If I understand correctly, majority of the time is spent on running commands like:

go list -e -compiler=gc -tags=ignore_autogenerated -installsuffix= '-f={{.Dir}} {{.ImportPath}} {{.Root}} {{.Goroot}} {{if .Error}}{{.Error}}{{end}}'  -- k8s.io/gengo/types
go list -e -compiler=gc -tags=ignore_autogenerated -installsuffix= '-f={{.Dir}} {{.ImportPath}} {{.Root}} {{.Goroot}} {{if .Error}}{{.Error}}{{end}}'  -- k8s.io/gengo/examples/set-gen

Each of this command run by hand takes ~50ms on my machine and they seem to be executed almost linearly according to perf and with Hotspot to analyze:
Selection_342

Those are triggered by

gengo/parser/parse.go

Line 595 in de9496d

buildPkg, err := b.context.Import(filepath.ToSlash(dir), cwd, mode)
.

I played a bit with x/tools/go/packages and I was actually able to reproduce the output of Import() command using packages.Load(). You can find those experiments here: https://github.com/kinvolk/gengo/commit/ce5df5f6a0db2b239a5bbdae96784d07f2626c66

However, this turns out to be slower than original code, as it seems packages.Load() invokes other binaries like cgo, compile, asm, link etc. There is a related upstream issue here: golang/go#31087.

At this point, I guess there are 2 things to try:

  • Add new API to be able to import packages in a bulk. Right now there is only AddDir() or Import(). Making them take a list of packages or adding new methods which take a list of packages would be good.
  • Try paralleling calls to import implementation (either Import() or packages.Load()) together with bulk import, it may speed things up.

I think before we try more things, it would also be nice to rewrite the whole thing to replace map operations with function calls etc. This would allow to write plain uncached version of the parser, then add caching as an independent module, to keep it separate from other code.

@palnabarun
Copy link
Member

/assign @palnabarun @nikhita

@vincepri
Copy link
Member Author

Hey @nikhita @palnabarun, anything I can do to help make this happen? We're hitting a roadblock in Cluster API (as @invidian pointed out above), which is slowing down our verification scripts as well.

@lavalamp
Copy link
Member

Just a note on testing if you make major surgery-- you should run the k8s code generation steps in the main kubernetes repo using the proposed changes to make sure nothing breaks.

I might be able to squeeze small PR reviews in but I still don't have cycles to offer any major help.

ironcladlou added a commit to ironcladlou/hypershift that referenced this issue Oct 28, 2021
@ironcladlou
Reduce API docs generation time by 97%
d1d7e25 
Optimize the API docs generation by disabling the use of Go modules
when running the generation tool. This is a workaround for performance
issues in the upstream libraries:

    kubernetes/gengo#147
    kubernetes/code-generator#69

Before this workaround:

    make api-docs  93.87s user 171.98s system 426% cpu 1:02.30 total

After the workaround:

    make api-docs  2.91s user 0.83s system 135% cpu 2.752 total

The hack seems worth the 97% improvement for iterating on docs.
ironcladlou added a commit to ironcladlou/hypershift that referenced this issue Oct 28, 2021
@ironcladlou
Reduce API docs generation time by 97%
c6ebf1f 
Optimize the API docs generation by disabling the use of Go modules
when running the generation tool. This is a workaround for performance
issues in the upstream libraries:

- kubernetes/gengo#147
- kubernetes/code-generator#69

Before this workaround:

    make api-docs  93.87s user 171.98s system 426% cpu 1:02.30 total

After the workaround:

    make api-docs  2.91s user 0.83s system 135% cpu 2.752 total

The hack seems worth the 97% improvement for iterating on docs.
@ironcladlou ironcladlou mentioned this issue Oct 28, 2021
Merged
ironcladlou added a commit to ironcladlou/hypershift that referenced this issue Oct 28, 2021
@ironcladlou
Reduce API docs generation time by 97%
a1d4f1d 
Optimize the API docs generation by disabling the use of Go modules
when running the generation tool. This is a workaround for performance
issues in the upstream libraries:

- kubernetes/gengo#147
- kubernetes/code-generator#69

Before this workaround:

    make api-docs  93.87s user 171.98s system 426% cpu 1:02.30 total

After the workaround:

    make api-docs  2.91s user 0.83s system 135% cpu 2.752 total

The hack seems worth the 97% improvement for iterating on docs.
@fmartingr fmartingr mentioned this issue Dec 24, 2022
Merged
fmartingr added a commit to mattermost/mattermost-operator that referenced this issue Dec 24, 2022
@fmartingr
Use custom GOPATH in github actions (#328)
a45854b 
workarounds kubernetes/gengo#147
gabrieljackson pushed a commit to mattermost/mattermost-operator that referenced this issue Mar 21, 2023
@spirosoik @fmartingr
@mattermost-build @toninis @phoinixgrr
Migrate from CircleCI to Github Actions (#324)
02ac79d 
* Migrate from CircleCI to Github Actions

We gradually moving away from CirleCI to Github Actions as we are consolidating
the CIs we use. In parallel we are introducing `trivy` in the PR level
and we block the PR about the number of critical vulnerabilities.

Ticket: https://mattermost.atlassian.net/browse/CLD-4711

* [fix] Remove CircleCI

* [fix] exclude vendor from lint

* [fix] golang version to 1.19

* Use custom GOPATH in github actions (#328)

workarounds kubernetes/gengo#147

* [fix] kind cluster wait to be created

* [fix] go cache dependency path

* [fix] update kind verson and k8s

* fix(ci): Add patch for MySQL operator on steup_test.sh

* test: Add temp SSH session for debug

* fixed fatal: detected dubious ownership issue with git checkout

* test on local runners

* upgrade percona to 5.7.35

* test in mac since I am now desperate

* try ubuntu-20.04

* test kind cluster from github actions

* 1.24 k8s version

* upgrade mysql-operator and use helm

* test with Percona 8.0

* remove upterm

* add commented out tests

* test CVE upload results

* Update .github/workflows/ci.yml

Co-authored-by: Akis Maziotis <[email protected]>

* Update .github/workflows/cd.yml

Co-authored-by: Akis Maziotis <[email protected]>

* naming convention . Trivy fix

* fix docker login

* fix sarif output

* continue on error for now for trivy results

* always upload since there is no failure for now

* allow specifying a mysql database version

* Set mysql version only for mysql database type

* ignore trivy result

* checking lesser retry interval

this way logs should not get disk filled in github runner

* even longer...?

* try running in 4core machine

---------

Co-authored-by: Felipe Martin <[email protected]>
Co-authored-by: Mattermost Build <[email protected]>
Co-authored-by: Antonis Stamatiou <[email protected]>
Co-authored-by: Akis Maziotis <[email protected]>
Co-authored-by: Felipe Martin <[email protected]>
@thockin
Copy link
Member

thockin commented Jan 28, 2024

Will be fixed in v2 (#259)

@thockin thockin closed this as completed Jan 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@palnabarun palnabarun

@nikhita nikhita

Labels
lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@tbe @lavalamp @vincepri @thockin @palnabarun @nikhita @invidian @k8s-ci-robot @fejta-bot