Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Graphviz_jll #71

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Use Graphviz_jll #71

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Project.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ AbstractTrees = "1520ce14-60c1-5f80-bbc7-55ef81b5835c"
CodecZlib = "944b1d66-785c-5afd-91f1-9de20f533193"
EnumX = "4e289a0a-7415-4d19-859d-a7e5c4648b56"
FlameGraphs = "08572546-2f56-4bcf-ba4e-bab62c3a3f89"
Graphviz_jll = "3c863552-8265-54e4-a6dc-903eb78fde85"
Libdl = "8f399da3-3557-5675-b5ff-fb832c97cbdb"
OrderedCollections = "bac558e1-5e72-5ebc-8fee-abe8a469f55d"
Profile = "9abbd945-dff8-562f-b5e8-e1ebf5ef1b79"
Expand All @@ -20,6 +21,7 @@ AbstractTrees = "0.3, 0.4"
CodecZlib = "0.7"
EnumX = "1"
FlameGraphs = "0.2, 1"
Graphviz_jll = "2.50.0"
OrderedCollections = "1.1"
ProgressMeter = "1.7"
ProtoBuf = "1"
Expand Down
4 changes: 0 additions & 4 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,6 @@ PProf.Allocs.pprof()

For more usage examples see the pprof docs: https://github.com/google/pprof/blob/master/doc/README.md

## Dependencies
- [Graphviz](https://www.graphviz.org/)
- In order to use pprof's web graph view (which is one of the best parts of pprof), you need to have graphviz installed. Click the above link to install.

## Usage
```julia
help?> pprof
Expand Down
5 changes: 4 additions & 1 deletion src/PProf.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ using ProtoBuf
using OrderedCollections
using CodecZlib
import pprof_jll
import Graphviz_jll

using Profile: clear

Expand Down Expand Up @@ -378,7 +379,9 @@ function refresh(; webhost::AbstractString = "localhost",
relative_percentages_flag = ui_relative_percentages ? "-relative_percentages" : ""

proc[] = pprof_jll.pprof() do pprof_path
open(pipeline(`$pprof_path -http=$webhost:$webport $relative_percentages_flag $file`))
Graphviz_jll.dot() do dot_path
open(pipeline(`$pprof_path -http=$webhost:$webport $relative_percentages_flag $file`))
end
end
Comment on lines 381 to 385
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this package requires Julia v1.6, you could use the recommended thread-safe syntax

run(`$(mungify_exe()) $arguments`)

instead of

mungify_exe() do exe
    run(`$exe $arguments`)
end

but I don't understand what this is doing: dot_path isn't used. Is it used only to merge the environments?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we just need to instead dot into the path, since it is going to be called by pprof

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bit convoluted, but can do something like

pprof = addenv(pprof_jll.pprof(), Graphviz_jll.JLLWrappers.LIBPATH_env=>Graphviz_jll.LIBPATH[], "PATH"=>Graphviz_jll.PATH[])
open(pipeline(`$pprof -http=$webhost:$webport $relative_percentages_flag $file`))

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not enough to just modify PATH, because shared libs used by dot must also be found.

I did the change the way I did it because it was the easiest and quickest way for me to achieve what I needed. I was not aware of the new syntax. I simply extended what was there (in particular it seems to me that any thread safety issues are already present and not just introduced by this PR?)

the patch by @giordano seems interesting, but I don't understand it right now -- e.g. shouldn't the "LIBPATH" and PATH content be merged with what is already there instead of overriding it? Or perhaps this is somehow done automatically? This may be obvious to you, but sadly not me :-/.

As it is, I don't have the time and energy to understand and figure all this out, then test it again etc. So if you want it to use that method I think it makes more sense if someone else (@giordano?) opens a PR with it. Feel free to close this PR then!

In the meantime, this patch allowed me to use PProf.jl and that in turn allowed me to tune some code substantially using the Memory Allocation Profiler. Thanks !!!

end

Expand Down