Issue 13085043: code review 13085043: misc/pprof: pprof http used with net/http/pprof not wor...
Descriptionmisc/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 #MessagesTotal messages: 18
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
Sign in to reply to this message.
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
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, alex.brainman@gmail.com (cc: bradfitz@golang.org, golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
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?
Sign in to reply to this message.
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
Sign in to reply to this message.
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?
Sign in to reply to this message.
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
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, alex.brainman@gmail.com, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
@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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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... >
Sign in to reply to this message.
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!
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** 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>
Sign in to reply to this message.
|
