|
|
|
misc/pprof: pprof http used with net/http/pprof not working on windows/amd64
Removed posix assumptions in temporary file generation
Removed curl dependence
Changed opening of svg file
These must now work including symbol resolution.
[1] go tool pprof <prog_name> http://.../debug/pprof/profile
[2] go tool pprof http://.../debug/pprof/profile
Fixes 6177.
Patch Set 1 #Patch Set 2 : diff -r 06593199ff39 https://code.google.com/p/go #Patch Set 3 : diff -r 06593199ff39 https://code.google.com/p/go #Patch Set 4 : diff -r ddd49fd9241d https://code.google.com/p/go #
Total comments: 14
Patch Set 5 : diff -r 037a28ab0725 https://code.google.com/p/go #Patch Set 6 : diff -r 037a28ab0725 https://code.google.com/p/go #
Total comments: 1
Patch Set 7 : diff -r 037a28ab0725 https://code.google.com/p/go #Total messages: 18
|
shiv
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
|
12 years, 4 months ago (2013年08月17日 12:14:52 UTC) #1 |
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
Your CL description should tell us what CL does. "pprof http used with net/http/pprof not working on windows/amd64" is not it. Please, change it. Also, this function does not work on my system because I don't have curl. I suspect many other windows users will be in a similar situation. How hard would be for us to run small Go program to do what curl does? Perhaps it won't be accepted. But maybe we should try it? Thank you. Alex
Hello golang-dev@googlegroups.com, alex.brainman@gmail.com (cc: bradfitz@golang.org, golang-dev@googlegroups.com), Please take another look.
On 2013年08月22日 04:11:17, shiv wrote: > Hello mailto:golang-dev@googlegroups.com, mailto:alex.brainman@gmail.com (cc: > mailto:bradfitz@golang.org, mailto:golang-dev@googlegroups.com), > > Please take another look. I couple of specific questions to someone who knows pprof/perl better. * Is it intended to persist profile file under dir pointed by $PPROF_TMPDIR when such a env variable exists? My CL is using a temp file that does not persist. * In response to Alex's comment about curl on windows, I have replaced it with LWP perl module. Is LWP basic enough?
On 2013年08月22日 04:14:50, shiv wrote: > ... It is better (if you can say that :0). But it still fails: C:\go\path\mine\src\issue6177>go tool pprof issue6177.exe http://localhost:6161/debug/pprof/profile Gathering CPU profile from http://localhost:6161/debug/pprof/profile?seconds=30 for 30 seconds to C:\DOCUME~1\brainman\LOCALS~1\Temp\qk9nGB6xac Be patient... Failed to get profile: http://localhost:6161/debug/pprof/profile?seconds=30 90! Could it be your test have no time (doCPUIntensiveThings) to send profile? Alex
On 2013年08月23日 02:55:16, brainman wrote: > > Gathering CPU profile from http://localhost:6161/debug/pprof/profile?seconds=30 > for 30 seconds to > C:\DOCUME~1\brainman\LOCALS~1\Temp\qk9nGB6xac > Be patient... > Failed to get profile: http://localhost:6161/debug/pprof/profile?seconds=30 90! > > Could it be your test have no time (doCPUIntensiveThings) to send profile? > The timeout value has not changed from original version. This should be neither better nor worse. This is also not windows related which the current bug is about. As long as there is no regression, can we agree to keep the timeout handling separate from this issue? Or I can increase the timeout to "some" higher value. Say 1.5 times the current. My test was *not* CPU intensive. In your CPU intensive test, was the MAXPROCS set to >1 so that profiling goroutine as well gets CPU time?
On 2013年08月23日 04:08:38, shiv wrote: > > My test was *not* CPU intensive. In your CPU intensive test, ... I just used your test http://play.golang.org/p/ohboNwlL3c from https://code.google.com/p/go/issues/detail?id=6177. > ... was the MAXPROCS > set to >1 so that profiling goroutine as well gets CPU time? Now that I have added runtime.GOMAXPROCS(2) at the start, everything works as expected. Unfortunately I am not familiar with Perl enough to review your code. Lets wait for an expert. Thank you very much. Alex
https://codereview.appspot.com/13085043/diff/11001/misc/pprof File misc/pprof (right): https://codereview.appspot.com/13085043/diff/11001/misc/pprof#newcode3032 misc/pprof:3032: my $response = $ua->get( $url ); remove spaces inside parens https://codereview.appspot.com/13085043/diff/11001/misc/pprof#newcode3034 misc/pprof:3034: my $cmdline = $response->content; before it only read one line (until \n), but now it slurps the whole content of the page. is this URL only a single line? to be safe, you might want to do: $cmdline =~ s/\n.*//s; https://codereview.appspot.com/13085043/diff/11001/misc/pprof#newcode3120 misc/pprof:3120: my $response = $lwp->request( $req ); use spaces, not tabs. remove spaces inside parens. https://codereview.appspot.com/13085043/diff/11001/misc/pprof#newcode3121 misc/pprof:3121: trailing whitespace https://codereview.appspot.com/13085043/diff/11001/misc/pprof#newcode3237 misc/pprof:3237: my $response = $ua->get($url); spaces instead of tabs here and next few lines https://codereview.appspot.com/13085043/diff/11001/misc/pprof#newcode3241 misc/pprof:3241: open(OUTFILE, ">$tmp_profile" ); remove space before paren https://codereview.appspot.com/13085043/diff/11001/misc/pprof#newcode3243 misc/pprof:3243: print OUTFILE ($response->content); remove parens
PTAL https://codereview.appspot.com/13085043/diff/11001/misc/pprof File misc/pprof (right): https://codereview.appspot.com/13085043/diff/11001/misc/pprof#newcode3032 misc/pprof:3032: my $response = $ua->get( $url ); On 2013年08月23日 21:01:57, bradfitz wrote: > remove spaces inside parens Done. https://codereview.appspot.com/13085043/diff/11001/misc/pprof#newcode3034 misc/pprof:3034: my $cmdline = $response->content; On 2013年08月23日 21:01:57, bradfitz wrote: > > is this URL only a single line? > > to be safe, you might want to do: > > $cmdline =~ s/\n.*//s; Yes it is single line. Suggested change done. https://codereview.appspot.com/13085043/diff/11001/misc/pprof#newcode3120 misc/pprof:3120: my $response = $lwp->request( $req ); On 2013年08月23日 21:01:57, bradfitz wrote: > use spaces, not tabs. remove spaces inside parens. Done. https://codereview.appspot.com/13085043/diff/11001/misc/pprof#newcode3121 misc/pprof:3121: On 2013年08月23日 21:01:57, bradfitz wrote: > trailing whitespace Done. https://codereview.appspot.com/13085043/diff/11001/misc/pprof#newcode3237 misc/pprof:3237: my $response = $ua->get($url); On 2013年08月23日 21:01:57, bradfitz wrote: > spaces instead of tabs here and next few lines Done. https://codereview.appspot.com/13085043/diff/11001/misc/pprof#newcode3241 misc/pprof:3241: open(OUTFILE, ">$tmp_profile" ); On 2013年08月23日 21:01:57, bradfitz wrote: > remove space before paren Done. https://codereview.appspot.com/13085043/diff/11001/misc/pprof#newcode3243 misc/pprof:3243: print OUTFILE ($response->content); On 2013年08月23日 21:01:57, bradfitz wrote: > remove parens Done.
Hello golang-dev@googlegroups.com, alex.brainman@gmail.com, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
@Alex, I noticed that "web" command was not working and have fixed this as well. Fix (windows specific) is to start svg file with default viewer associated in the OS instead of previous failed attempt to start a browser.
On 2013年08月26日 03:23:56, shiv wrote: > @Alex, > I noticed that "web" command was not working and have fixed this as well. > Fix (windows specific) is to start svg file with default viewer associated in > the OS instead of previous failed attempt to start a browser. I don't have "svg associated viewer", so it is asking me to select a viewer anyway. Also svg file is empty all the time. I suggest you fight with that in a different CL. Alex
https://codereview.appspot.com/13085043/diff/22001/misc/pprof File misc/pprof (right): https://codereview.appspot.com/13085043/diff/22001/misc/pprof#newcode109 misc/pprof:109: my $CURL = "curl"; if you don't use $CURL anymore, you should remove this line.
Tangentially related, but would the team be open to a Go implementation of this tool if someone contributed it? The Perl version has made me cringe the few times I've had to hack it to make it work with weird HTTP server configurations.
Yes please. On Aug 27, 2013 10:06 AM, <kamil.kisiel@gmail.com> wrote: > Tangentially related, but would the team be open to a Go implementation > of this tool if someone contributed it? The Perl version has made me > cringe the few times I've had to hack it to make it work with weird HTTP > server configurations. > > https://codereview.appspot.**com/13085043/<https://codereview.appspot.com/130... >
PTAL As suggested by Alex "web" command support is removed for a separate CL. curl references are completely removed. On 2013年08月27日 17:07:40, bradfitz wrote: > Yes please. > On Aug 27, 2013 10:06 AM, <mailto:kamil.kisiel@gmail.com> wrote: > > > Tangentially related, but would the team be open to a Go implementation > > of this tool if someone contributed it? The Perl version has made me > > cringe the few times I've had to hack it to make it work with weird HTTP > > server configurations. > > Would love to see pure Go version. Perl fixes were always only couple of lines away Perl fixes were couple of lines away and need for pure go version wasn't needly badly enough!
LGTM
*** Submitted as https://code.google.com/p/go/source/detail?r=66ba9277d461 *** misc/pprof: pprof http used with net/http/pprof not working on windows/amd64 Removed posix assumptions in temporary file generation Removed curl dependence Changed opening of svg file These must now work including symbol resolution. [1] go tool pprof <prog_name> http://.../debug/pprof/profile [2] go tool pprof http://.../debug/pprof/profile Fixes 6177. R=golang-dev, alex.brainman, bradfitz, kamil.kisiel CC=golang-dev https://codereview.appspot.com/13085043 Committer: Brad Fitzpatrick <bradfitz@golang.org>