-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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/pprof: list not accurate in 1.9 #21697
Comments
CC @rauls5382 |
I am seeing this on linux as well when using a Docker container running Alpine and/or Debian. What version of Go are you using (go version)? go version go1.9.1 linux/amd64 What operating system and processor architecture are you using (go env)? Using Alpine3.6 in a Docker Container GOARCH="amd64" Code to reproduce the bug? https://github.com/ardanlabs/gotraining/tree/master/topics/go/profiling/memcpu What did you do?
What did you expect to see? I expected to only see algOne
I also expected to see the allocations (both Flat and Cum) on line 83
|
This looks like bad line number information, probably from @ardan-bkennedy, can you try this with current master? It's possible this has already been fixed. |
I tried with 1.9.2 this evening and the cum number showed but not the flat. That was on Darwin. I’ll try master this weekend. Thank You! |
@aclements I just tried with master and the same thing. The allocation does not show up in the flat column for line 83.
|
It does look like the bug of only showing the code in question has been fixed! |
Sorry, I think there are multiple issues going on here and I was confused. @ardan-bkennedy, the output you just posted looks 100% correct to me. It does look like we fixed the mis-attribution of the allocation to the import line, which was clearly a bug. @ardan-bkennedy, from another issue it sounded like you were interested in having that fixed in 1.9. If so, I'm not positive it passes the bar for back-porting, but if you can bisect down to which commit fixed it (or, @mdempsky, maybe you know off the top of your head?) we could consider it. |
That call is inlined and I expected it to be flat because in version 1.8.x this was the case. So this is a change in the way the tooling is reporting allocations then? Are you saying it is more acurate to show it this way instead of the old way? I don't care so much about the answer only so I can be accurate when teaching. I have material and videos on 1.8.x that teach the inlining will cause the value to the flat. |
I checked the release notes for 1.9, it does not document this change in the way allocations are reported. Since the escape analysis report |
Right. Reporting it as flat was a bug in 1.8 caused by Go's lack of inlining information. That was fixed in 1.9 and now the report is accurate (or, at least, it will be accurate in 1.10; clearly attributing the import line was not accurate and we might back-port the fix if we know what it was). Inlining is an implementation detail. Previously you would have gotten a different report if you compiled with certain debug flags, or if
Now you can simplify that. :) |
Perfect! Thanks but I have a question. If I have a function like NewBuffer which can be inlined and the value never escapes the heap, because the calling function does not cause the allocation. Then I would expect to see no allocation at all? I am going to test this. |
I think this leads to more confusion. I can make the call to How would you describe what the |
I don't think you should involve inlining at all in the explanation. This is purely a question of whether or not there's a heap allocation. The compiler optimizes heap allocations into stack allocations all the time for lots of reasons, and this is just another case of that. The profiler shows you the truth of heap allocations because you want to optimize what actually gets allocated in places where it actually matters, and you shouldn't be guessing about what the compiler will or won't do in subtle cases (that's why there's a profiler :)
The |
The only fix that comes to mind is CL 64470 (7c8a961), but I'd be surprised if that was relevant to a simple bytes.NewBuffer call. |
I will let it go but I think this is a mistake. The profiler should show the allocations accurately. If the function was inlined, then the allocation should show as flat. In this case, the heap allocations did occur directly on that line of code. Thanks for everything you do! |
As a compiler developer, I would expect cmd/pprof should report the allocation the same way, whether or not a function was inlined. Put differently, in an ideal world, I expect function inlining to be a purely toolchain-internal decision that users are isolated from. (In the real world this often doesn't happen, but that's at least what we strive for.) I'm not really a cmd/pprof expert. Are you saying you expect cmd/pprof output to vary depending on whether the function was inlined or not? (I would disagree with this, but then I'd be interested to know why you think that.) Or are you saying when the function is not inlined, the allocation does show as flat, so it's an error that inlining is changing this? (I would agree this is a bug.) |
I wanted the profile to show flat numbers based on the decisions made during escape analysis. I believe it makes it easier to reason about the profile. I appreciate that profiling and escape analysis are two separate concerns but they do come together when trying to understand a profile. A profile can only show you what allocated and the escape analysis can tell you why. When a function is inlined, the value is technically not own by that function anymore, but the calling function. This is why I wanted to see the allocation as flat inside the calling function. Technically it's the calling function that is causing the allocation because the calling function owns the creation of the value (since the function call was inlined). I just believe it makes the profiles easier to read. This is how it has been for a long time, so this change is confusing. It would have been nice to see some release notes on this change and to have some discusion on the decision before it was made. The ship has sailed, so no more need to discuss it. |
The new behavior, with accurate call stacks even in the presence of inlining, is correct. Working as intended. |
After talking to Sameer I agree. I’ve adjusted my teaching accordingly. Thank You. |
What version of Go are you using (go version)?
go version go1.9 darwin/amd64
What operating system and processor architecture are you using (go env)?
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/bill/code/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/_v/4r515ktx08g5yr6dgkxhfyfr0000gn/T/go-build016620058=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
Code to reproduce the bug?
https://github.com/ardanlabs/gotraining/tree/master/topics/go/profiling/memcpu
What did you do?
The allocation for
input
is not being reported in pprof underflat
but the escape analysis shows that the call tobytes.NewBuffer
was inlinedWhat did you expect to see?
The text was updated successfully, but these errors were encountered: