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

cmd/compile: rsc/compilebench -alloc is broken #18641

Closed
mdempsky opened this issue Jan 12, 2017 · 10 comments
Closed

cmd/compile: rsc/compilebench -alloc is broken #18641

mdempsky opened this issue Jan 12, 2017 · 10 comments
Milestone

Comments

@mdempsky
Copy link
Member

Using rsc/compilebench's -alloc flag, all compiles currently report "0 B/op 0 allocs/op". Presumably something in cmd/compile changed that broke this. We should either revert that change, or update rsc/compilebench accordingly.

@mdempsky mdempsky added this to the Go1.9 milestone Jan 12, 2017
@mdempsky
Copy link
Member Author

This appears to be because runtime/pprof switched to writing binary pprof files, so compilebench's code to parse the # TotalAlloc and # Mallocs lines no longer work.

/cc @matloob

@josharian
Copy link
Contributor

Just hit this myself. Sadness. (I wonder whether anyone else was parsing the pprof output to get memstats and will also be impacted by this change.)

I don't see an obvious way to add ignored to the binary pprof output, and I really don't want to embed a protobuf decoder in compilebench anyway.

I propose instead that we add a -memstats=<file> flag to cmd/compile. When provided, on exit, we call runtime.ReadMemStats and write the JSON-encoded results to <file>.

compilebench will have to probe whether the compiler accepts the flag. If yes, it uses it and reads the result. If no, it omits it and attempts to read the pprof output. There will be a stretch of commits in Go 1.8 and the beginning of Go 1.9 for which compilebench forever cannot emit alloc info. (Or in dire straights you can patch runtime/pprof/pprof.go's WriteHeapProfile to call writeHeap(w, 1) instead of writeHeap(w, 0).)

How does that sound?

@minux
Copy link
Member

minux commented Jan 20, 2017 via email

@josharian
Copy link
Contributor

I assume that at some point @matloob will want to eliminate the old code, but as an interim measure, that'd be really useful. Particularly if it could go in for 1.8--it is a tiny, safe change--but I'm not going to hold my breath for that.

@minux minux modified the milestones: Go1.8, Go1.9 Jan 20, 2017
@minux
Copy link
Member

minux commented Jan 20, 2017 via email

@bradfitz
Copy link
Contributor

Can you prepare a non-scary CL before Monday?

@josharian
Copy link
Contributor

False alarm. While preparing a CL, I accidentally read the documentation (oops). CL on its way.

@josharian
Copy link
Contributor

gopherbot down? CL 35484

@minux
Copy link
Member

minux commented Jan 20, 2017 via email

@josharian
Copy link
Contributor

But those who have the problem have a one line fix available. They'd have to do the same thing with a GODEBUG envvar.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants