JuliaPerf / PProf.jl

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

Fix bugs in PProf.Allocs rendering #70

Closed NHDaly closed 1 year ago

NHDaly commented 1 year ago

Fixes https://github.com/JuliaPerf/PProf.jl/issues/69.

There were two bugs that this PR fixes:

  1. This one was a dumb mistake: we were incorrectly using the function name instead of the method instance to uniquely ID a function, meaning that we could only have a single method specialization in the profile at a time, i think!? Dumb mistake!
  2. This one was not dumb at all: Apparently it's not safe to use a StackFrame as a key in a Dict!?!? This fixes that! We use the frame's string representation instead, which seems to be unique.

So now the profile from #69 looks correct. Phew!

codecov-commenter commented 1 year ago

Codecov Report

Merging #70 (71127d0) into master (5f48183) will decrease coverage by 0.29%. The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
- Coverage   96.90%   96.61%   -0.30%     
==========================================
  Files           3        3              
  Lines         291      295       +4     
==========================================
+ Hits          282      285       +3     
- Misses          9       10       +1     
Impacted Files Coverage Δ
src/Allocs.jl 92.00% <85.71%> (-1.16%) :arrow_down:
src/PProf.jl 99.20% <100.00%> (+0.01%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

NHDaly commented 1 year ago

And I added a test for Bug 1, which I confirmed failed on master. It's pretty ridiculous that it took us this long to notice this pretty basic bug 😓

[ Info: Writing output to /var/folders/fd/hymwsmhj1zd27lwlftv_yb2r0000gn/T/jl_sROx2Tqfiv.pb.gz
Multiple method specializations: Test Failed at /Users/nathandaly/.julia/dev/PProf/test/Allocs.jl:26
  Expression: "foo(::Float64)" in prof.string_table
   Evaluated: "foo(::Float64)" in ["", "allocs", "count", "size", "bytes", "heap", "maybe_record_alloc_to_profile", "/Users/nathandaly/builds/julia-1.8/src/./gc-alloc-profiler.h", "jl_gc_alloc_", "/Users/nathandaly/builds/julia-1.8/src/./julia_internal.h"  …  "jl_repl_entrypoint", "Alloc: Vector{Int64}", "nothing", "type", "Vector{Int64}", "Alloc: Vector{Float64}", "Vector{Float64}", "jl_f_tuple", "Alloc: Tuple{Vector{Int64}, Vector{Float64}}", "Tuple{Vector{Int64}, Vector{Float64}}"]