Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2089)

Issue 6443115: code review 6443115: pprof: add contention profiling (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 10 months ago by dvyukov
Modified:
8 years, 8 months ago
Reviewers:
CC:
rsc, dfc, minux1, r, gobot, golang-dev, remyoudompheng
Visibility:
Public.

Description

pprof: add goroutine blocking profiling The profiler collects goroutine blocking information similar to Google Perf Tools. You may see an example of the profile (converted to svg) attached to http://code.google.com/p/go/issues/detail?id=3946 The public API changes are: +pkg runtime, func BlockProfile([]BlockProfileRecord) (int, bool) +pkg runtime, func SetBlockProfileRate(int) +pkg runtime, method (*BlockProfileRecord) Stack() []uintptr +pkg runtime, type BlockProfileRecord struct +pkg runtime, type BlockProfileRecord struct, Count int64 +pkg runtime, type BlockProfileRecord struct, Cycles int64 +pkg runtime, type BlockProfileRecord struct, embedded StackRecord

Patch Set 1 #

Patch Set 2 : diff -r 66e0219bd117 https://go.googlecode.com/hg/ #

Total comments: 3

Patch Set 3 : diff -r ab60a1c1e9b4 https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r a14ef77ab98c https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 5 : diff -r ab60a1c1e9b4 https://go.googlecode.com/hg/ #

Patch Set 6 : diff -r ab60a1c1e9b4 https://go.googlecode.com/hg/ #

Total comments: 13

Patch Set 7 : diff -r e2f74da67564 https://go.googlecode.com/hg/ #

Total comments: 8

Patch Set 8 : diff -r c568561320a3 https://go.googlecode.com/hg/ #

Patch Set 9 : diff -r c568561320a3 https://go.googlecode.com/hg/ #

Patch Set 10 : diff -r c568561320a3 https://go.googlecode.com/hg/ #

Patch Set 11 : diff -r ac1b735e8753 https://go.googlecode.com/hg/ #

Total comments: 6

Patch Set 12 : diff -r 248e11862ed5 https://go.googlecode.com/hg/ #

Patch Set 13 : diff -r 248e11862ed5 https://go.googlecode.com/hg/ #

Total comments: 2

Patch Set 14 : diff -r 248e11862ed5 https://go.googlecode.com/hg/ #

Patch Set 15 : diff -r 248e11862ed5 https://go.googlecode.com/hg/ #

Total comments: 6

Patch Set 16 : diff -r d2b0f85e5d9f https://go.googlecode.com/hg/ #

Patch Set 17 : diff -r d2b0f85e5d9f https://go.googlecode.com/hg/ #

Total comments: 6

Patch Set 18 : diff -r b4d17e91718d https://go.googlecode.com/hg/ #

Patch Set 19 : diff -r 03190651924e https://go.googlecode.com/hg/ #

Total comments: 3

Patch Set 20 : diff -r 03190651924e https://go.googlecode.com/hg/ #

Total comments: 4

Patch Set 21 : diff -r 03190651924e https://go.googlecode.com/hg/ #

Patch Set 22 : diff -r 03190651924e https://go.googlecode.com/hg/ #

Patch Set 23 : diff -r 2aef5548a9cf https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+350 lines, -52 lines) Patch
M src/cmd/go/test.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +12 lines, -0 lines 0 comments Download
M src/cmd/go/testflag.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 2 chunks +4 lines, -0 lines 0 comments Download
M src/pkg/net/http/pprof/pprof.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +4 lines, -0 lines 0 comments Download
M src/pkg/runtime/chan.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 13 chunks +36 lines, -0 lines 0 comments Download
M src/pkg/runtime/debug.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +25 lines, -0 lines 0 comments Download
M src/pkg/runtime/mprof.goc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 8 chunks +102 lines, -19 lines 0 comments Download
M src/pkg/runtime/pprof/pprof.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 21 4 chunks +65 lines, -0 lines 0 comments Download
M src/pkg/runtime/runtime.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +3 lines, -0 lines 0 comments Download
M src/pkg/runtime/runtime.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +37 lines, -0 lines 0 comments Download
M src/pkg/runtime/sema.goc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 5 chunks +34 lines, -14 lines 0 comments Download
M src/pkg/runtime/signal_linux_arm.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +4 lines, -10 lines 0 comments Download
M src/pkg/testing/testing.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 3 chunks +24 lines, -9 lines 0 comments Download

Messages

Total messages: 62
dfc
This change is causing me to salivate. Do you have any measurements of any performance ...
8 years, 10 months ago (2012-08-13 07:55:37 UTC) #1
dvyukov
On Mon, Aug 13, 2012 at 10:55 AM, <dave@cheney.net> wrote: > This change is causing ...
8 years, 10 months ago (2012-08-13 08:06:59 UTC) #2
dfc
> Doesn't this involve struct copy? In this case yes. As I was suggesting using ...
8 years, 10 months ago (2012-08-13 08:13:47 UTC) #3
dfc
Ignore all my comments, you were correct. http://codereview.appspot.com/6443115/diff/1002/src/pkg/runtime/debug.go File src/pkg/runtime/debug.go (right): http://codereview.appspot.com/6443115/diff/1002/src/pkg/runtime/debug.go#newcode70 src/pkg/runtime/debug.go:70: return r.Stack0[0:] ...
8 years, 10 months ago (2012-08-13 08:14:03 UTC) #4
dvyukov
Hello dave@cheney.net (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
8 years, 10 months ago (2012-08-14 21:10:19 UTC) #5
dfc
Thank you. I'm super excited to see this feature committed. Please review the comments below, ...
8 years, 10 months ago (2012-08-15 07:15:53 UTC) #6
dvyukov
Updated API diff: +pkg net/http/pprof, func ContentionProfileRate(http.ResponseWriter, *http.Request) +pkg runtime, func ContentionProfile([]ContentionProfileRecord) (int, bool) +pkg ...
8 years, 10 months ago (2012-08-15 09:52:36 UTC) #7
dvyukov
PTAL http://codereview.appspot.com/6443115/diff/5/src/cmd/go/test.go File src/cmd/go/test.go (right): http://codereview.appspot.com/6443115/diff/5/src/cmd/go/test.go#newcode114 src/cmd/go/test.go:114: are complete. On 2012/08/15 07:15:53, dfc wrote: > ...
8 years, 10 months ago (2012-08-15 09:55:45 UTC) #8
dvyukov
http://codereview.appspot.com/6443115/diff/2006/src/pkg/net/http/pprof/pprof.go File src/pkg/net/http/pprof/pprof.go (right): http://codereview.appspot.com/6443115/diff/2006/src/pkg/net/http/pprof/pprof.go#newcode145 src/pkg/net/http/pprof/pprof.go:145: func ContentionProfileRate(w http.ResponseWriter, r *http.Request) { This is somewhat ...
8 years, 10 months ago (2012-08-15 10:03:32 UTC) #9
dfc
Looks good. Some minor comments. http://codereview.appspot.com/6443115/diff/5/src/pkg/runtime/debug.go File src/pkg/runtime/debug.go (right): http://codereview.appspot.com/6443115/diff/5/src/pkg/runtime/debug.go#newcode159 src/pkg/runtime/debug.go:159: // a prefix of ...
8 years, 10 months ago (2012-08-15 10:17:27 UTC) #10
dvyukov
http://codereview.appspot.com/6443115/diff/2006/src/pkg/runtime/mprof.goc File src/pkg/runtime/mprof.goc (right): http://codereview.appspot.com/6443115/diff/2006/src/pkg/runtime/mprof.goc#newcode281 src/pkg/runtime/mprof.goc:281: runtime·contentionevent(int64 cycles) On 2012/08/15 10:17:28, dfc wrote: > Does ...
8 years, 10 months ago (2012-08-15 10:48:54 UTC) #11
r
On Wed, Aug 15, 2012 at 2:52 AM, Dmitry Vyukov <dvyukov@google.com> wrote: > Updated API ...
8 years, 10 months ago (2012-08-15 16:15:43 UTC) #12
dvyukov
On 2012/08/15 16:15:43, r wrote: > On Wed, Aug 15, 2012 at 2:52 AM, Dmitry ...
8 years, 10 months ago (2012-08-15 16:27:16 UTC) #13
dvyukov
Added a fix that removes uninteresting users of semaphores from the profile. In particular I've ...
8 years, 10 months ago (2012-08-19 08:58:52 UTC) #14
dvyukov
Added chan blocking profiling.
8 years, 10 months ago (2012-08-19 09:19:11 UTC) #15
remyoudompheng
On 2012/08/19 08:58:52, dvyukov wrote: > Added a fix that removes uninteresting users of semaphores ...
8 years, 10 months ago (2012-08-19 09:26:18 UTC) #16
dvyukov
On Sun, Aug 19, 2012 at 1:26 PM, <remyoudompheng@gmail.com> wrote: > On 2012/08/19 08:58:52, dvyukov ...
8 years, 10 months ago (2012-08-19 09:54:47 UTC) #17
dvyukov
Added ContentionProfileRate support.
8 years, 10 months ago (2012-08-19 10:27:56 UTC) #18
dvyukov
On 2012/08/19 09:19:11, dvyukov wrote: > Added chan blocking profiling. no select's for now
8 years, 10 months ago (2012-08-19 10:28:10 UTC) #19
gobot
R=rsc (assigned by rsc)
8 years, 9 months ago (2012-08-31 20:01:07 UTC) #20
dfc
ping.
8 years, 9 months ago (2012-09-11 00:49:59 UTC) #21
rsc
Overall seems okay but there are things to clean up. http://codereview.appspot.com/6443115/diff/7026/src/cmd/go/test.go File src/cmd/go/test.go (right): http://codereview.appspot.com/6443115/diff/7026/src/cmd/go/test.go#newcode112 ...
8 years, 9 months ago (2012-09-17 21:08:07 UTC) #22
dfc
Can you please remail this, it doesn't apply cleanly any more. Also, there may be ...
8 years, 9 months ago (2012-09-18 17:19:49 UTC) #23
dvyukov
On Mon, Sep 17, 2012 at 2:08 PM, <rsc@golang.org> wrote: > Overall seems okay but ...
8 years, 9 months ago (2012-09-18 19:49:10 UTC) #24
rsc
I meant just the flag name but probably the other API names should match.
8 years, 9 months ago (2012-09-18 19:54:25 UTC) #25
dvyukov
http://codereview.appspot.com/6443115/diff/7026/src/pkg/runtime/mprof.goc File src/pkg/runtime/mprof.goc (right): http://codereview.appspot.com/6443115/diff/7026/src/pkg/runtime/mprof.goc#newcode290 src/pkg/runtime/mprof.goc:290: if(rate > cycles && runtime·fastrand1()%rate > cycles) On 2012/09/17 ...
8 years, 9 months ago (2012-09-18 20:27:23 UTC) #26
dvyukov
On 2012/09/18 17:19:49, dfc wrote: > Can you please remail this, it doesn't apply cleanly ...
8 years, 9 months ago (2012-09-18 21:06:39 UTC) #27
dfc
I haven't had a case to use it apart from the issues that you raised ...
8 years, 9 months ago (2012-09-18 21:10:28 UTC) #28
dvyukov
http://codereview.appspot.com/6443115/diff/27013/src/pkg/runtime/runtime.c File src/pkg/runtime/runtime.c (right): http://codereview.appspot.com/6443115/diff/27013/src/pkg/runtime/runtime.c#newcode363 src/pkg/runtime/runtime.c:363: runtime∕pprof·runtime_cyclesPerSecond(int64 res) How about this? On my machine where ...
8 years, 9 months ago (2012-09-18 21:28:41 UTC) #29
dfc
I don't know if that will be reliable enough. Even with the performance governor enabled, ...
8 years, 9 months ago (2012-09-18 21:32:59 UTC) #30
dvyukov
I would expect TSC to tick with constant freq regardless of TurboBoost and C states. ...
8 years, 9 months ago (2012-09-18 21:34:48 UTC) #31
dvyukov
Otherwise I would prefer to drop this at all, pprof assumes 2GHz by default which ...
8 years, 9 months ago (2012-09-18 21:36:08 UTC) #32
dvyukov
On 2012/09/18 19:54:25, rsc wrote: > I meant just the flag name but probably the ...
8 years, 9 months ago (2012-09-18 22:16:59 UTC) #33
minux1
http://codereview.appspot.com/6443115/diff/27013/src/pkg/runtime/runtime.c File src/pkg/runtime/runtime.c (right): http://codereview.appspot.com/6443115/diff/27013/src/pkg/runtime/runtime.c#newcode363 src/pkg/runtime/runtime.c:363: runtime∕pprof·runtime_cyclesPerSecond(int64 res) On 2012/09/18 21:28:42, dvyukov wrote: > How ...
8 years, 9 months ago (2012-09-19 09:25:59 UTC) #34
rsc
LGTM Please wait for r to take a look at the new API + testing ...
8 years, 9 months ago (2012-09-20 18:47:36 UTC) #35
r
http://codereview.appspot.com/6443115/diff/36001/src/cmd/go/test.go File src/cmd/go/test.go (right): http://codereview.appspot.com/6443115/diff/36001/src/cmd/go/test.go#newcode119 src/cmd/go/test.go:119: aims to sample an average of one blocking event ...
8 years, 9 months ago (2012-09-20 18:57:57 UTC) #36
rsc
time spent blocking is measured in cpu cycles (perhaps confusingly) if the rate is set ...
8 years, 9 months ago (2012-09-20 19:47:15 UTC) #37
dvyukov
https://codereview.appspot.com/6443115/diff/36001/src/cmd/go/test.go File src/cmd/go/test.go (right): https://codereview.appspot.com/6443115/diff/36001/src/cmd/go/test.go#newcode120 src/cmd/go/test.go:120: CPU cycles. By default all blocking events are recorded. ...
8 years, 9 months ago (2012-09-20 20:01:29 UTC) #38
dvyukov
On 2012/09/20 19:47:15, rsc wrote: > time spent blocking is measured in cpu cycles (perhaps ...
8 years, 9 months ago (2012-09-20 20:11:07 UTC) #39
rsc
> potentially BlockProfileRace may be measured in nanoseconds, but then I > need to convert ...
8 years, 9 months ago (2012-09-20 20:12:52 UTC) #40
dvyukov
On Thu, Sep 20, 2012 at 1:12 PM, Russ Cox <rsc@golang.org> wrote: > > potentially ...
8 years, 9 months ago (2012-09-20 20:14:02 UTC) #41
dvyukov
PTAL Now BlockProfileRate is in ns. Fixed comments. Added runtime·tickspersecond(void), it caches the value so ...
8 years, 9 months ago (2012-09-20 22:31:10 UTC) #42
r
Now I'm bothered by the idea of a rate being measure in units of time ...
8 years, 9 months ago (2012-09-21 04:50:02 UTC) #43
r
https://codereview.appspot.com/6443115/diff/37006/src/cmd/go/test.go File src/cmd/go/test.go (right): https://codereview.appspot.com/6443115/diff/37006/src/cmd/go/test.go#newcode120 src/cmd/go/test.go:120: spent blocked. By default all blocking events are recorded. ...
8 years, 9 months ago (2012-09-21 04:57:37 UTC) #44
rsc
> src/pkg/runtime/debug.go:144: // one blocking event per BlockProfileRate > nanoseconds spent blocked. > what does ...
8 years, 9 months ago (2012-09-21 05:06:12 UTC) #45
dvyukov
PTAL https://codereview.appspot.com/6443115/diff/37006/src/cmd/go/test.go File src/cmd/go/test.go (right): https://codereview.appspot.com/6443115/diff/37006/src/cmd/go/test.go#newcode120 src/cmd/go/test.go:120: spent blocked. By default all blocking events are ...
8 years, 9 months ago (2012-09-22 03:51:57 UTC) #46
dvyukov
Is it OK to submit now? On 2012/09/22 03:51:57, dvyukov wrote: > PTAL > > ...
8 years, 9 months ago (2012-09-26 21:49:28 UTC) #47
minux1
On 2012/09/26 21:49:28, dvyukov wrote: > Is it OK to submit now? The only problem ...
8 years, 9 months ago (2012-09-27 09:39:30 UTC) #48
dvyukov
Can't we use nanotime() or something like that on ARM? It may be slower, but ...
8 years, 9 months ago (2012-09-27 18:53:32 UTC) #49
dfc
Let's try with nanotime() on arm in the spirit of getting this in.
8 years, 9 months ago (2012-09-29 04:32:23 UTC) #50
dvyukov
PTAL I've made cputicks() use nanotime() on arm. https://codereview.appspot.com/6443115/diff/54001/src/pkg/runtime/signal_linux_arm.c File src/pkg/runtime/signal_linux_arm.c (right): https://codereview.appspot.com/6443115/diff/54001/src/pkg/runtime/signal_linux_arm.c#newcode209 src/pkg/runtime/signal_linux_arm.c:209: return ...
8 years, 8 months ago (2012-10-01 13:18:10 UTC) #51
minux1
LGTM. https://codereview.appspot.com/6443115/diff/54001/src/pkg/runtime/signal_linux_arm.c File src/pkg/runtime/signal_linux_arm.c (right): https://codereview.appspot.com/6443115/diff/54001/src/pkg/runtime/signal_linux_arm.c#newcode208 src/pkg/runtime/signal_linux_arm.c:208: runtime·cputicks() { could you please add some comments ...
8 years, 8 months ago (2012-10-01 15:30:07 UTC) #52
minux1
LGTM.
8 years, 8 months ago (2012-10-01 15:30:23 UTC) #53
dvyukov
PTAL https://codereview.appspot.com/6443115/diff/54001/src/pkg/runtime/signal_linux_arm.c File src/pkg/runtime/signal_linux_arm.c (right): https://codereview.appspot.com/6443115/diff/54001/src/pkg/runtime/signal_linux_arm.c#newcode208 src/pkg/runtime/signal_linux_arm.c:208: runtime·cputicks() { On 2012/10/01 15:30:07, minux wrote: > ...
8 years, 8 months ago (2012-10-01 15:46:45 UTC) #54
minux1
LGTM. (seems Chrome already foreseen this and sent one earlier.)
8 years, 8 months ago (2012-10-01 15:50:32 UTC) #55
dvyukov
On 2012/10/01 15:50:32, minux wrote: > LGTM. (seems Chrome already foreseen this and sent one ...
8 years, 8 months ago (2012-10-01 15:52:25 UTC) #56
minux1
Please update CL description (roughly s/Contention/Block(ing)/).
8 years, 8 months ago (2012-10-01 16:01:43 UTC) #57
rsc
https://codereview.appspot.com/6443115/diff/61005/src/pkg/net/http/pprof/pprof.go File src/pkg/net/http/pprof/pprof.go (right): https://codereview.appspot.com/6443115/diff/61005/src/pkg/net/http/pprof/pprof.go#newcode151 src/pkg/net/http/pprof/pprof.go:151: // BlockProfileRate gets or sets runtime.BlockProfileRate. I don't think ...
8 years, 8 months ago (2012-10-01 20:11:20 UTC) #58
dvyukov
On 2012/10/01 16:01:43, minux wrote: > Please update CL description (roughly s/Contention/Block(ing)/). Done.
8 years, 8 months ago (2012-10-02 05:54:20 UTC) #59
dvyukov
PTAL https://codereview.appspot.com/6443115/diff/61005/src/pkg/net/http/pprof/pprof.go File src/pkg/net/http/pprof/pprof.go (right): https://codereview.appspot.com/6443115/diff/61005/src/pkg/net/http/pprof/pprof.go#newcode151 src/pkg/net/http/pprof/pprof.go:151: // BlockProfileRate gets or sets runtime.BlockProfileRate. On 2012/10/01 ...
8 years, 8 months ago (2012-10-02 05:57:56 UTC) #60
rsc
LGTM Thanks for your patience.
8 years, 8 months ago (2012-10-05 20:16:00 UTC) #61
dvyukov
8 years, 8 months ago (2012-10-06 08:56:45 UTC) #62
*** Submitted as http://code.google.com/p/go/source/detail?r=b19c6b32c8da ***

pprof: add goroutine blocking profiling
The profiler collects goroutine blocking information similar to Google Perf
Tools.
You may see an example of the profile (converted to svg) attached to
http://code.google.com/p/go/issues/detail?id=3946
The public API changes are:
+pkg runtime, func BlockProfile([]BlockProfileRecord) (int, bool)
+pkg runtime, func SetBlockProfileRate(int)
+pkg runtime, method (*BlockProfileRecord) Stack() []uintptr
+pkg runtime, type BlockProfileRecord struct
+pkg runtime, type BlockProfileRecord struct, Count int64
+pkg runtime, type BlockProfileRecord struct, Cycles int64
+pkg runtime, type BlockProfileRecord struct, embedded StackRecord

R=rsc, dave, minux.ma, r
CC=gobot, golang-dev, r, remyoudompheng
http://codereview.appspot.com/6443115
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b