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.

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

x/tools/go/packages: LoadSyntax performance after LoadMode option split into Need constants #31930

Closed
codeactual opened this issue May 9, 2019 · 3 comments

Comments

@codeactual
Copy link

commented May 9, 2019

What version of Go are you using (go version)?

$ go version
go version go1.12.5 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

My environment:

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/var/go-build"
GOEXE=""
GOFLAGS="-mod=vendor"
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/var/dev/megaton/gocode"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="0"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build130407124=/tmp/go-build -gno-record-gcc-switches"

Travis CI environment:

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/travis/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/travis/gopath"
GOPROXY=""
GORACE=""
GOROOT="/home/travis/.gimme/versions/go1.12.5.linux.amd64"
GOTMPDIR=""
GOTOOLDIR="/home/travis/.gimme/versions/go1.12.5.linux.amd64/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build294531687=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I upgraded from golang/tools@f727bef to golang/tools@fe54fb3 and noticed slower performance and out-of-memory (especially when running with CGO_ENABLED=1 for go test -race) errors in a non-public refactoring program that relies on packages.Load. I then bisected until finding golang/tools@dbeab5a.

To assist my testing I used a small program to repro which just calls packages.Load on some public packages.

In a later section I've included the output from both my local machine and a Travis build.

What did you expect to see?

I expected to see that commit golang/tools@dbeab5a did not significantly change the memory profile of programs that heavily use packages.Load (with Tests: true and Mode: LoadSyntax).

What did you see instead?

My results (local)

golang/tools@45dd101, one commit prior to bisect search result:

: cat 45dd101d8784.top | head -20
File: repro
Type: alloc_space
Time: May 9, 2019 at 12:03am (UTC)
Showing nodes accounting for 170.73MB, 96.71% of 176.54MB total
Dropped 46 nodes (cum <= 0.88MB)
      flat  flat%   sum%        cum   cum%
         0     0%     0%   159.41MB 90.30%  golang.org/x/tools/go/packages.(*loader).loadPackage
         0     0%     0%   159.41MB 90.30%  golang.org/x/tools/go/packages.(*loader).loadRecursive.func1
         0     0%     0%   159.41MB 90.30%  sync.(*Once).Do
         0     0%     0%   158.91MB 90.01%  golang.org/x/tools/go/packages.(*loader).loadRecursive
         0     0%     0%   153.74MB 87.09%  golang.org/x/tools/go/packages.(*loader).loadRecursive.func1.1
         0     0%     0%   152.67MB 86.48%  golang.org/x/tools/go/gcexportdata.Read
         0     0%     0%   152.67MB 86.48%  golang.org/x/tools/go/packages.(*loader).loadFromExportData
         0     0%     0%   146.98MB 83.26%  bytes.(*Buffer).grow
  146.98MB 83.26% 83.26%   146.98MB 83.26%  bytes.makeSlice
         0     0% 83.26%   146.48MB 82.97%  bytes.(*Buffer).ReadFrom
         0     0% 83.26%   143.37MB 81.21%  io/ioutil.readAll
         0     0% 83.26%   142.86MB 80.92%  io/ioutil.ReadAll
    0.50MB  0.28% 83.54%    11.81MB  6.69%  golang.org/x/tools/go/internal/gcimporter.(*iimporter).doDecl
         0     0% 83.54%    11.81MB  6.69%  golang.org/x/tools/go/internal/gcimporter.(*importReader).obj

golang/tools@dbeab5a, result of bisect:

: cat dbeab5af4b8d.top | head -20
File: repro
Type: alloc_space
Time: May 9, 2019 at 12:03am (UTC)
Showing nodes accounting for 2647.39MB, 95.13% of 2782.91MB total
Dropped 158 nodes (cum <= 13.91MB)
      flat  flat%   sum%        cum   cum%
         0     0%     0%  1890.71MB 67.94%  go/types.(*Checker).checkFiles
         0     0%     0%  1863.15MB 66.95%  go/types.(*Checker).Files
         0     0%     0%  1842.55MB 66.21%  golang.org/x/tools/go/packages.(*loader).loadPackage
         0     0%     0%  1827.82MB 65.68%  golang.org/x/tools/go/packages.(*loader).loadRecursive.func1
         0     0%     0%  1795.45MB 64.52%  sync.(*Once).Do
         0     0%     0%  1762.59MB 63.34%  golang.org/x/tools/go/packages.(*loader).loadRecursive
         0     0%     0%  1730.81MB 62.19%  golang.org/x/tools/go/packages.(*loader).loadRecursive.func1.1
         0     0%     0%  1410.51MB 50.68%  go/types.(*Checker).rawExpr
   19.50MB   0.7%   0.7%  1302.49MB 46.80%  go/types.(*Checker).stmt
         0     0%   0.7%  1299.35MB 46.69%  go/types.(*Checker).multiExpr
         0     0%   0.7%  1298.87MB 46.67%  go/types.(*Checker).stmtList
    2.16MB 0.077%  0.78%  1275.78MB 45.84%  go/types.(*Checker).exprInternal
         0     0%  0.78%  1227.46MB 44.11%  go/types.(*Checker).funcBody
         0     0%  0.78%  1188.49MB 42.71%  go/types.(*Checker).funcDecl.func1

golang/tools@e31d365, newest version at test time:

: head -20 e31d36578abb.top 
File: repro
Type: alloc_space
Time: May 9, 2019 at 1:22am (UTC)
Showing nodes accounting for 2610.92MB, 94.57% of 2760.70MB total
Dropped 167 nodes (cum <= 13.80MB)
      flat  flat%   sum%        cum   cum%
         0     0%     0%  1846.72MB 66.89%  go/types.(*Checker).checkFiles
         0     0%     0%  1822.69MB 66.02%  go/types.(*Checker).Files
    0.50MB 0.018% 0.018%  1790.19MB 64.85%  golang.org/x/tools/go/packages.(*loader).loadPackage
         0     0% 0.018%  1774.66MB 64.28%  golang.org/x/tools/go/packages.(*loader).loadRecursive.func1
         0     0% 0.018%  1746.55MB 63.26%  sync.(*Once).Do
         0     0% 0.018%  1714.51MB 62.10%  golang.org/x/tools/go/packages.(*loader).loadRecursive
         0     0% 0.018%  1691.43MB 61.27%  golang.org/x/tools/go/packages.(*loader).loadRecursive.func1.1
         0     0% 0.018%  1389.83MB 50.34%  go/types.(*Checker).rawExpr
         0     0% 0.018%  1284.40MB 46.52%  go/types.(*Checker).multiExpr
      21MB  0.76%  0.78%  1269.81MB 46.00%  go/types.(*Checker).stmt
         0     0%  0.78%  1266.73MB 45.88%  go/types.(*Checker).stmtList
    2.60MB 0.094%  0.87%  1254.36MB 45.44%  go/types.(*Checker).exprInternal
         0     0%  0.87%  1195.09MB 43.29%  go/types.(*Checker).funcBody
         0     0%  0.87%  1155.35MB 41.85%  go/types.(*Checker).funcDecl.func1
My results (on Travis CI)

It seems like the scope of LoadSyntax has significantly changed now that Load* constants are now based on the more granular Need*, especially with regard to use of go/types (based on the profiles above).

If there is a combination of Need* options which can restore similar performance with the same returned package data, I'm very interested to use them instead of LoadSyntax.

@gopherbot gopherbot added this to the Unreleased milestone May 9, 2019

@andybons

This comment has been minimized.

Copy link
Member

commented May 13, 2019

jirfag added a commit to golangci/tools that referenced this issue Sep 9, 2019
go/packages: allow types loading without NeedDeps
Before separating Load* into Need* we could use LoadSyntax
to get types information by loading inital packages
from source code and loading it's direct dependencies from export data.
It was broken when separation was introduced and before this commit
everything was loading from source code what resulted into slow
loading times. This commit fixes it.

Also, do backwards-incompatible fix of definition of deprecated
LoadImports and LoadAllSyntax.

Improve an internal error message
"internal error: nil Pkg importing x from y": replace it with
"internal error: package x without types was imported from y".

Remove packages.NeedDeps request for loading in tests
TestLoadTypesBits and TestContainsOverlayXTest.

Fixes golang/go#31752, fixes golang/go#33077, fixes golang/go#32814,
      fixes golang/go#31699, golang/go#31930
jirfag added a commit to golangci/tools that referenced this issue Sep 9, 2019
go/packages: allow types loading without NeedDeps
Before separating Load* into Need* we could use LoadSyntax
to get types information by loading inital packages
from source code and loading it's direct dependencies from export data.
It was broken when separation was introduced and before this commit
everything was loading from source code what resulted into slow
loading times. This commit fixes it.

Also, do backwards-incompatible fix of definition of deprecated
LoadImports and LoadAllSyntax.

Improve an internal error message
"internal error: nil Pkg importing x from y": replace it with
"internal error: package x without types was imported from y".

Remove packages.NeedDeps request for loading in tests
TestLoadTypesBits and TestContainsOverlayXTest.

Fixes golang/go#31752, fixes golang/go#33077, fixes golang/go#32814,
      fixes golang/go#31699, fixes golang/go#31930
@gopherbot

This comment has been minimized.

Copy link

commented Sep 9, 2019

Change https://golang.org/cl/186337 mentions this issue: go/packages: allow types loading without NeedDeps

@andybons

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

jirfag added a commit to golangci/tools that referenced this issue Sep 10, 2019
go/packages: allow types loading without NeedDeps
Before separating Load* into Need* we could use LoadSyntax
to get types information by loading inital packages
from source code and loading it's direct dependencies from export data.
It was broken when separation was introduced and before this commit
everything was loading from source code what resulted into slow
loading times. This commit fixes it.

Also, do backwards-incompatible fix of definition of deprecated
LoadImports and LoadAllSyntax.

Improve an internal error message
"internal error: nil Pkg importing x from y": replace it with
"internal error: package x without types was imported from y".

Remove packages.NeedDeps request for loading in tests
TestLoadTypesBits and TestContainsOverlayXTest.

Fixes golang/go#31752, fixes golang/go#33077, fixes golang/go#32814,
      fixes golang/go#31699, fixes golang/go#31930
clintjedwards added a commit to clintjedwards/tools that referenced this issue Sep 19, 2019
go/packages: allow types loading without NeedDeps
Before separating Load* into Need* we could use LoadSyntax
to get types information by loading inital packages
from source code and loading it's direct dependencies from export data.
It was broken when separation was introduced and before this commit
everything was loading from source code what resulted into slow
loading times. This commit fixes it.

Also, do backwards-incompatible fix of definition of deprecated
LoadImports and LoadAllSyntax.

Improve an internal error message
"internal error: nil Pkg importing x from y": replace it with
"internal error: package x without types was imported from y".

Remove packages.NeedDeps request for loading in tests
TestLoadTypesBits and TestContainsOverlayXTest.

Fixes golang/go#31752, fixes golang/go#33077, fixes golang/go#32814,
          fixes golang/go#31699, fixes golang/go#31930

Change-Id: I416e3c1035d555d67039e45a4fdd1deb9b2184ef
GitHub-Last-Rev: 2e3a46e
GitHub-Pull-Request: golang#139
Reviewed-on: https://go-review.googlesource.com/c/tools/+/186337
Reviewed-by: Michael Matloob <matloob@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.