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: always log GC details in-memory #23815

Open
heschik opened this Issue Feb 13, 2018 · 9 comments

Comments

Projects
None yet
6 participants
@heschik
Contributor

heschik commented Feb 13, 2018

Whenever a user reports a potential issue with the GC, the first thing we ask for is a GC trace (GODEBUG=gctrace=1). We rarely get it.

gctrace=1 is cheap enough to leave on all the time, but spamming stderr is poor form. Instead, the runtime could log it in memory somewhere, either as text or structured data, and expose it in a /debug handler or something. Then it'd be much easier to pull on demand.

This might also be useful for GODEBUG=sched{detail,trace} but I haven't seen that come up as often.

@dgryski

This comment has been minimized.

Contributor

dgryski commented Feb 14, 2018

@dhananjay92

This comment has been minimized.

Member

dhananjay92 commented Mar 1, 2018

Anyone working on this? If not, I can give this a shot.

Prototype: 78ec37e

It dumps runtime.MemStats (which has almost all the things printed by gctrace=1) in an internal circular buffer and exposes it via runtime.ReadAllMemStats(). It doesn't yet expose anything on /debug handler.

Should it take buffer size from GODEBUG?

@heschik

This comment has been minimized.

Contributor

heschik commented Mar 1, 2018

Nobody's working on it that I know of, so feel free. But if MemStats was all we wanted, we could just poll ReadMemStats. Usually we want to know the per-phase CPU/wall time, and that's not in there, just the STW pause wall time.

Maybe the easiest way to accomplish this would be to add the extra information to MemStats. That would be covered by the Go compatibility promise though...

@dhananjay92

This comment has been minimized.

Member

dhananjay92 commented Mar 1, 2018

Maybe the easiest way to accomplish this would be to add the extra information to MemStats. That would be covered by the Go compatibility promise though

I am assuming there's a typo here. Because the comment says "we cannot change MemStats because of backward compatibility". Or am I missing something?

@heschik

This comment has been minimized.

Contributor

heschik commented Mar 1, 2018

I presume (didn't verify) that's referring to a field that changed meaning internally, and we can't propagate that change to the public structure because it could break existing code. Adding fields should be fine.

@dhananjay92

This comment has been minimized.

Member

dhananjay92 commented Mar 1, 2018

There's an explicit check for the size that would fail on adding a new field. It was added in 2010 : dc9a3b2#diff-17dbb5a92bd43f80a1909a1814e2d42eR82 .

AIUI, back then part of code was in C and part in Go; the check was to make sure both sizes matched up. But that's not the case anymore. So, can we safely remove that check?

@RLH

This comment has been minimized.

Contributor

RLH commented Mar 1, 2018

@RLH

This comment has been minimized.

Contributor

RLH commented Mar 2, 2018

@gopherbot

This comment has been minimized.

gopherbot commented Mar 2, 2018

Change https://golang.org/cl/98335 mentions this issue: runtime: Expose MemStats for previous GC cycles.

@andybons andybons added the NeedsFix label Mar 7, 2018

@andybons andybons added this to the Go1.11 milestone Mar 7, 2018

@RLH RLH modified the milestones: Go1.11, Go1.12 Jul 11, 2018

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