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: heap profiles can have negative inuse values #19311

Closed
aclements opened this issue Feb 27, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@aclements
Copy link
Member

commented Feb 27, 2017

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

go version go1.8 linux/amd64

What did you do?

Add the following to runtime.mprof_GC() and run all.bash:

--- a/src/runtime/mprof.go
+++ b/src/runtime/mprof.go
@@ -220,6 +220,10 @@ func mprof_GC() {
                mp.alloc_bytes += mp.prev_alloc_bytes
                mp.free_bytes += mp.prev_free_bytes
 
+               if mp.frees > mp.allocs {
+                       throw("frees > allocs")
+               }
+
                mp.prev_allocs = mp.recent_allocs
                mp.prev_frees = mp.recent_frees
                mp.prev_alloc_bytes = mp.recent_alloc_bytes

What did you expect to see?

No crashes

What did you see instead?

Sometimes heap profiles of very small heaps report more frees than allocs, resulting in negative inuse counts. (Though the issue corrects itself on the next GC cycle.)

Why?

In 5b7497f, I moved the mProf_GC() call to just after GC starts the world, reasoning that it was okay if a few allocations got accounted to an earlier cycle. However, if sweeping frees any objects in the window between starting the world and calling mProf_GC(), those frees can be accounted to an earlier cycle. If an object doesn't live across any cycles, it's then possible for the free to get accounted to an earlier cycle than the allocation, resulting in a temporary negative inuse count.

@aclements aclements added this to the Go1.9 milestone Feb 27, 2017

@aclements aclements self-assigned this Feb 27, 2017

@aclements

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2017

Unfortunately, the obvious solution of acquiring proflock before starting the world and then calling mprof_GC() doesn't work because it's not safe to start the world while holding proflock (it can allocate, among other things). This may require a generation counter to do right.

@gopherbot

This comment has been minimized.

Copy link

commented Mar 2, 2017

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

@gopherbot

This comment has been minimized.

Copy link

commented Mar 2, 2017

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

gopherbot pushed a commit that referenced this issue Mar 31, 2017

runtime: pull heap profile cycle into a type
Currently memRecord has the same set of four fields repeated three
times. Pull these into a type and use this type three times. This
cleans up and simplifies the code a bit and will make it easier to
switch to a globally tracked heap profile cycle for #19311.

Change-Id: I414d15673feaa406a8366b48784437c642997cf2
Reviewed-on: https://go-review.googlesource.com/37713
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rick Hudson <rlh@golang.org>

@gopherbot gopherbot closed this in eee85fc Mar 31, 2017

lparth added a commit to lparth/go that referenced this issue Apr 13, 2017

runtime: pull heap profile cycle into a type
Currently memRecord has the same set of four fields repeated three
times. Pull these into a type and use this type three times. This
cleans up and simplifies the code a bit and will make it easier to
switch to a globally tracked heap profile cycle for golang#19311.

Change-Id: I414d15673feaa406a8366b48784437c642997cf2
Reviewed-on: https://go-review.googlesource.com/37713
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rick Hudson <rlh@golang.org>

lparth added a commit to lparth/go that referenced this issue Apr 13, 2017

runtime: snapshot heap profile during mark termination
Currently we snapshot the heap profile just *after* mark termination
starts the world because it's a relatively expensive operation.
However, this means any alloc or free events that happen between
starting the world and snapshotting the heap profile can be accounted
to the wrong cycle. In the worst case, a free can be accounted to the
cycle before the alloc; if the heap is small, this can result
temporarily in a negative "in use" count in the profile.

Fix this without making STW more expensive by using a global heap
profile cycle counter. This lets us split up the operation into a two
parts: 1) a super-cheap snapshot operation that simply increments the
global cycle counter during STW, and 2) a more expensive cleanup
operation we can do after starting the world that frees up a slot in
all buckets for use by the next heap profile cycle.

Fixes golang#19311.

Change-Id: I6bdafabf111c48b3d26fe2d91267f7bef0bd4270
Reviewed-on: https://go-review.googlesource.com/37714
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rick Hudson <rlh@golang.org>

@golang golang locked and limited conversation to collaborators Mar 31, 2018

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.