JuliaPerf / PProf.jl

Export Julia profiles to the pprof format
MIT License
155 stars 17 forks source link

usability improvements #9

Closed gdkrmr closed 4 years ago

gdkrmr commented 4 years ago
gdkrmr commented 4 years ago
gdkrmr commented 4 years ago

That should be all for now. Cool package!

NHDaly commented 4 years ago

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:

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 commented 4 years ago
  • [ ] 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 commented 4 years ago

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.

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.

NHDaly commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago
NHDaly commented 4 years ago

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