usability improvements

[gdkrmr]

• Runs the server in background
• restarts the server at every pprof() call.
• @pprof ex as an alias to Profile.clear(); @profile ex; pprof()

[vchuravy]

Thank you for the PR!

[NHDaly]

Haha :) yes, thank you for the PR, @gdkrmr! :)

Much appreciated!

[gdkrmr]

• imported Profile.clear, so it can be called as PProf.clear
• PProf.refresh(...) restarts the server
• pprof_proc -> PProf.proc, which is also a Ref now.
• Documentation.
• still unsure about the @pprof macro

[NHDaly]

Cool! I think this is a great improvement! :) Thanks again for opening the PR, @gdkrmr. :)

Pending my last comments, I'm happy to merge this.

[gdkrmr]

• the process is started by open(pipe(...)) now. this prints an extra error message in the terminal
  "Main binary filename not available."
  but works otherwise
• added unit tests for managing the subprocess.
• squashed all commits.

That should be all for now. Cool package!

[NHDaly]

this prints an extra error message in the terminal
"Main binary filename not available."
but works otherwise

I think this is because pprof wants you to also pass the name of the binary that the profile belongs to, so that it can use that for extra debug output or something. It gives the same warning if you just run pprof on the command line:

$ deps/usr/bin/pprof -http=localhost:23432 profile.pb.gz
Main binary filename not available.
Serving web UI on http://localhost:23432

One noticeable change is that now when you run pprof() it doesn't automatically open the server in the browser.. you have to copy/paste the link. I think this seems less than ideal... I can't yet figure out how to fix that.

Also, annoyingly, somehow the pprof process is still staying alive! It's not us; i've confirmed that the pprof process is running in our process group (which means it should be killed when we exit), but somehow it's jumping ship and staying alive, moving its parent process to 1. I'm not very familiar with unix land, but this sounds like some weird unix process stuff...

It's confusing because if i start a simple child process like this, it does get closed when I exit:

p = open(pipeline(`bash -c 'while true; do sleep 1 && echo "hi"; done'`))  # killed when we ctrl-D julia

but the go subprocess is not:

p = open(pipeline(`$(PProf.go_pprof) -http=localhost:23432 profile.pb.gz`))  # _NOT_ killed when we ctrl-D julia!

Some googling led me to this article, but i don't think this actually has anything to do with Go (unless the binary is somehow doing something to set itself to "please don't kill me!!" or something):
https://medium.com/@felixge/killing-a-child-process-and-all-of-its-children-in-go-54079af94773

• added unit tests for managing the subprocess.
• squashed all commits.

🙌 Thanks! :) They look great

So to summarize my remaining complaints:

• pprof() now doesn't open the webserver in the browser. I think it would be nice to do this (though if you already have it open, you maybe don't want to do that... Ideally we'd only do this the first time?
• The pprof binary is still outliving julia, which means you can't run PProf again without manually finding and killing that process (because it complains about bind: address already in use).

The second one seems problematic enough that we probably shouldn't merge this until it's fixed. :'( I'm sorry, @gdkrmr! Thanks for your really fast and great work on this; i think this will make the package more usable. :)

[NHDaly]

• The pprof binary is still outliving julia, which means you can't run PProf again without manually finding and killing that process (because it complains about bind: address already in use).

Oh, huh, apparently my understanding of unix process death was just totally wrong. Orphaned processes are reparented by default, not killed:
https://stackoverflow.com/questions/284325/how-to-make-child-process-die-after-parent-exits

The more you know!

And what's more, it doesn't sound like there's a good way to kill them by default, which is super lame. The best solutions people have are to have your child process poll the parent process and exit when the parent exits... But this sounds nontrivial.

Maybe a better approach will be to:

1. register an atexit hook to kill the child process, and
2. Allow pprof() to retry different random ports if the default port is occupied?

[gdkrmr]

pprof() now doesn't open the webserver in the browser. I think it would be nice to do this (though if you already have it open, you maybe don't want to do that... Ideally we'd only do this the first time?

I think the best behaviour is to not open the browser.

• Implementing this in a platform and browser independent way sounds tedious.
• Most terminals detect links anyway and you can just click on it.

Maybe a better approach will be to:

• register an atexit hook to kill the child process, and

agreed

• Allow pprof() to retry different random ports if the default port is occupied?

This seems like bad practice, an unsuspecting user may end up with dozens of pprof servers running.

• Another bad possibility would be querying who is occupying the port and killing that process, maybe with a forcekill=true option.
• Maybe make @pprof throw an error if the port is occupied (this has to be checked before running the profile):
  "The requested port is already busy, this may be due to a pprof server that did not exit, you may have to kill the process manually. You can start a server listening to another port: @pprof port=1234 ex"

[NHDaly]

For the second problem, i just wrote this kind of dumb utility, which maybe would be sufficient for our usecase?
https://github.com/NHDaly/ExitWithParent.jl

[gdkrmr]

For the second problem, i just wrote this kind of dumb utility, which maybe would be sufficient for our usecase?
https://github.com/NHDaly/ExitWithParent.jl

That is kind of cool :-), maybe a bit overkill.

[NHDaly]

Okay! I've addressed my concerns by just adding an atexit hook, and that seems to work pretty well! :)

If for some reason it doesn't work (like if julia crashes) the user can always just provide a different webport, so i think that's good enough! :)

Thanks for the brainstorming with me, @gdkrmr! And for this nice PR :)

[NHDaly]

• Oh, we should probably update the README as well, with the new usage

[NHDaly]

Alright, thanks @gdkrmr! :) I'm merging this now! :)