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
Closed

runtime: document MemStats better #15849

aclements opened this issue May 26, 2016 · 6 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@aclements
Copy link
Member

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 The path to resolution is known, but the work has not been done. label May 26, 2016
@rsc rsc modified the milestones: Go1.7Maybe, Go1.7 May 27, 2016
@hyangah
Copy link
Contributor

hyangah commented Jun 2, 2016

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

@mberhault
Copy link

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
Copy link
Contributor

Postponing to 1.8.

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

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

@aclements
Copy link
Member Author

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
Copy link

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

gopherbot pushed a commit that referenced this issue Sep 26, 2016
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>
@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.
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

7 participants