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

runtime: document MemStats better #15849

Closed
aclements opened this issue May 26, 2016 · 6 comments

Comments

Projects
None yet
7 participants
@aclements
Copy link
Member

commented May 26, 2016

The current runtime.MemStats documentation is terse and impenetrable. Questions about the meanings of these fields come up not infrequently. We should improve its documentation so that it actually explains what the fields mean as well as enough of the allocation model to understand their meanings and apply them to debugging.

/cc @RLH @hyangah

@aclements aclements added this to the Go1.7 milestone May 26, 2016

@aclements aclements self-assigned this May 26, 2016

@quentinmit quentinmit added the NeedsFix label May 26, 2016

@rsc rsc modified the milestones: Go1.7Maybe, Go1.7 May 27, 2016

@hyangah

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2016

additionally, would be nice if the documentation explains how they relate (or not relate) to pprof heap profile result.

@mberhault

This comment has been minimized.

Copy link

commented Jun 9, 2016

one example among many: from looking through mheap.go, it looks like none of the counters are decremented when memory is released to the system. Specifically, HeapIdle looks like it really should, and HeapSys may or may not depending on whether it's supposed to be "total memory obtained from the system", or "current mem ...".

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2016

Postponing to 1.8.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.8, Go1.7Maybe Jul 7, 2016

@gopherbot

This comment has been minimized.

Copy link

commented Sep 10, 2016

CL https://golang.org/cl/28972 mentions this issue.

@aclements

This comment has been minimized.

Copy link
Member Author

commented Sep 15, 2016

additionally, would be nice if the documentation explains how they relate (or not relate) to pprof heap profile result.

I'm not sure where to put this that would make sense and people would see it. We do document in runtime/pprof that profiles are as of the most recently completed GC. I've added a blurb to ReadMemStats to clarify that it's instantaneous and to contrast it with profiles, but I don't know if it'll do any good.

@gopherbot

This comment has been minimized.

Copy link

commented Sep 15, 2016

CL https://golang.org/cl/29270 mentions this issue.

gopherbot pushed a commit that referenced this issue Sep 26, 2016

runtime: disentangle next_gc from GC trigger
Back in Go 1.4, memstats.next_gc was both the heap size at which GC
would trigger, and the size GC kept the heap under. When we switched
to concurrent GC in Go 1.5, we got somewhat confused and made this
variable the trigger heap size, while gcController.heapGoal became the
goal heap size.

memstats.next_gc is exposed to the user via MemStats.NextGC, while
gcController.heapGoal is not. This is unfortunate because 1) the heap
goal is far more useful for diagnostics, and 2) the trigger heap size
is just part of the GC trigger heuristic, which means it wouldn't be
useful to an application even if it tried to use it.

We never noticed this mess because MemStats.NextGC is practically
undocumented. Now that we're trying to document MemStats, it became
clear that this field had diverged from its original usefulness.

Clean up this mess by shuffling things back around so that next_gc is
the goal heap size and the new (unexposed) memstats.gc_trigger field
is the trigger heap size. This eliminates gcController.heapGoal.

Updates #15849.

Change-Id: I2cbbd43b1d78bdf613cb43f53488bd63913189b7
Reviewed-on: https://go-review.googlesource.com/29270
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Rick Hudson <rlh@golang.org>

@gopherbot gopherbot closed this in f67c9de Sep 26, 2016

@golang golang locked and limited conversation to collaborators Sep 26, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.