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

Jump to bottom

runtime: heap profiles can have negative inuse values #19311

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

runtime: heap profiles can have negative inuse values #19311

aclements opened this issue Feb 27, 2017 · 3 comments
Labels
FrozenDueToAge
Milestone
Go1.9

Comments

@aclements
Copy link
Member

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

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

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

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Mar 31, 2017
@aclements
runtime: pull heap profile cycle into a type
3ebe7d7 
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>
lparth pushed a commit to lparth/go that referenced this issue Apr 13, 2017
@aclements @lparth
runtime: pull heap profile cycle into a type
9d78516 
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 pushed a commit to lparth/go that referenced this issue Apr 13, 2017
@aclements @lparth
runtime: snapshot heap profile during mark termination
107c87b 
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.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.9
Development

No branches or pull requests
2 participants
@aclements @gopherbot