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

unused: significant performance and memory usage regression between 2017.2.2 and 2019.1 #394

Closed
RaduBerinde opened this issue Jan 15, 2019 · 7 comments

Comments

@RaduBerinde
Copy link

For the CockroachDB repo, unused in 2017.2.2 used to take ~30s and use ~8GB RAM. In 2019.1 it takes ~100 seconds and over 16GB RAM. The memory usage is especially concerning because on systems with 16GB, it will swap and become terribly slow.

Below are some stats with /usr/bin/time -v between the two versions. This should be easily reproducible by just pulling the cockroach repo.

2019.1:
	Command being timed: "/go/src/honnef.co/go/tools/cmd/unused/unused --exported github.com/cockroachdb/cockroach/pkg/..."
	User time (seconds): 329.54
	System time (seconds): 16.17
	Percent of CPU this job got: 336%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 1:42.76
	Average shared text size (kbytes): 0
	Average unshared data size (kbytes): 0
	Average stack size (kbytes): 0
	Average total size (kbytes): 0
	Maximum resident set size (kbytes): 17910580                   <---------
	Average resident set size (kbytes): 0
	Major (requiring I/O) page faults: 0
	Minor (reclaiming a frame) page faults: 7681331
	Voluntary context switches: 204395
	Involuntary context switches: 39889
	Swaps: 0
	File system inputs: 0
	File system outputs: 23816
	Socket messages sent: 0
	Socket messages received: 0
	Signals delivered: 0
	Page size (bytes): 4096
	Exit status: 1


2017.2.2:
	Command being timed: "/go/src/honnef.co/go/tools/cmd/unused/unused --exported github.com/cockroachdb/cockroach/pkg/..."
	User time (seconds): 109.70
	System time (seconds): 7.69
	Percent of CPU this job got: 321%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 0:36.54
	Average shared text size (kbytes): 0
	Average unshared data size (kbytes): 0
	Average stack size (kbytes): 0
	Average total size (kbytes): 0
	Maximum resident set size (kbytes): 7792776          <------------
	Average resident set size (kbytes): 0
	Major (requiring I/O) page faults: 0
	Minor (reclaiming a frame) page faults: 3173957
	Voluntary context switches: 76253
	Involuntary context switches: 25920
	Swaps: 0
	File system inputs: 8
	File system outputs: 8680
	Socket messages sent: 0
	Socket messages received: 0
	Signals delivered: 0
	Page size (bytes): 4096
	Exit status: 1
@RaduBerinde
Copy link
Author

This is with go1.11.4 by the way.

Looks like the increased memory usage happened with commit a85e435.

@dominikh
Copy link
Owner

Unfortunately that is kind of to be expected. go/packages changes the way tests and their dependencies are handled. Tests require their own versions of dependencies (including the package under test). The old go/loader didn't do this, instead bundling both packages in one. The new go/packages works correctly. Unfortunately the consequence is that we're dealing with approximately twice the number of packages.

There are of course ways to improve unused's memory usage, and to deduplicate data from both packages, but that'll require some design changes in unused. And unused, right now, is a hot mess, being one of my first tool projects. It is due for a rewrite, but that rewrite is too far in the future to be a solution to these issues. I'll see what I can do in the short-term.

In the meantime, you can experiment with setting GOGC to a value lower than 100, to force more frequent GCs, which keeps the overall heap smaller, at the cost of increased CPU usage.

I will also push a change that disables running unused completely when its checks are being ignored.

2018-09-08-000451_1437x1135_scrot
2018-09-08-000518_1437x1135_scrot
2018-09-08-000528_1437x1135_scrot

@dominikh
Copy link
Owner

This should be easily reproducible by just pulling the cockroach repo

That is an unfortunate lie, considering the C dependencies, some of which seems to be bundled with cockroach and not available in package managers (at least I assume that roach is something internal to cockroachdb). CGO_ENABLED=0 doesn't seem to work for github.com/cockroachdb/cockroach/pkg/... either, as some packages do not have pure Go alternatives. Not the best reproduction case.

@RaduBerinde
Copy link
Author

That is an unfortunate lie, considering the C dependencies

Hm, you are right, sorry. You would need to first run make buildshort which will build all the C dependencies and such. Unfortunately there are a few tools required to build, listed here.

But anyway sounds like there is likely nothing special about the repo. I will experiment with GOGC.

@dominikh
Copy link
Owner

Do note that a large part of unused's memory usage comes from the use of whole program mode (-exported). Specifically, I am seeing these numbers:

unused
2017.2.2  Maximum resident set size (kbytes): 4805892
2019.1    Maximum resident set size (kbytes): 11720840

unused -exported
2017.2.2  Maximum resident set size (kbytes): 7777236
2019.1    Maximum resident set size (kbytes): 17137552

Normal operation does suffer from the same >2x increase, and we do have to fix that, but a temporary workaround would be to forego whole program mode, assuming the other changes in staticcheck 2019.1 are worth that compromise.

@RaduBerinde
Copy link
Author

Thanks, yeah I did notice that without --exported it's better. We do benefit from using --exported though.

dominikh added a commit that referenced this issue Jan 28, 2019
types.Implements is a relatively expensive operation, producing a lot of
garbage in lookupFieldOrMethod.

When I tested 2019.1 on github.com/cockroachdb/cockroach/pkg/...,
lookupFieldOrMethod allocated 23 GB of garbage.

Updates gh-394
dominikh added a commit that referenced this issue Jan 28, 2019
The standard types.Implements implementation produces a lot of garbage
in lookupFieldOrMethod. By using MethodSetCache and method sets, we can
avoid a lot of this garbage. This directly translates to lower runtimes
and lower peak memory usage.

Compared with 442643a, for std and cockroachdb, we get the following
numbers:

Total allocations
std:  6268 MB ->  4702 MB
cdb: 28398 MB -> 19048 MB

Peak rss
std:  4,556,404 ->  3,253,004
cdb: 17,403,616 -> 12,546,948

Time
std: 0:25.18 -> 0:21.84
cdb: 1:45.70 -> 1:30.74

Updates gh-394
dominikh added a commit that referenced this issue Jan 28, 2019
types.Implements is a relatively expensive operation, producing a lot of
garbage in lookupFieldOrMethod.

When I tested 2019.1 on github.com/cockroachdb/cockroach/pkg/...,
lookupFieldOrMethod allocated 23 GB of garbage.

Updates gh-394
dominikh added a commit that referenced this issue Jan 28, 2019
The standard types.Implements implementation produces a lot of garbage
in lookupFieldOrMethod. By using MethodSetCache and method sets, we can
avoid a lot of this garbage. This directly translates to lower runtimes
and lower peak memory usage.

Compared with 442643a, for std and cockroachdb, we get the following
numbers:

Total allocations
std:  6268 MB ->  4702 MB
cdb: 28398 MB -> 19048 MB

Peak rss
std:  4,556,404 ->  3,253,004
cdb: 17,403,616 -> 12,546,948

Time
std: 0:25.18 -> 0:21.84
cdb: 1:45.70 -> 1:30.74

Updates gh-394
@dominikh dominikh mentioned this issue Mar 20, 2019
@dominikh
Copy link
Owner

I'm going to merge this issue into #376. Once we finish the port, memory usage will be greatly reduced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants