timholy / SnoopCompile.jl

Provide insights about latency (TTFX) for Julia packages
https://timholy.github.io/SnoopCompile.jl/dev/
Other
309 stars 48 forks source link

Fix string cat in show method invalidations #268

Closed charleskawczynski closed 2 years ago

charleskawczynski commented 2 years ago

This PR fixes a small typo for concatenating strings in the show method for invalidations.

Thanks for the great package!

codecov[bot] commented 2 years ago

Codecov Report

Merging #268 (71ab836) into master (d18b722) will decrease coverage by 4.41%. The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #268      +/-   ##
==========================================
- Coverage   86.87%   82.46%   -4.42%     
==========================================
  Files          16       16              
  Lines        2004     1893     -111     
==========================================
- Hits         1741     1561     -180     
- Misses        263      332      +69     
Impacted Files Coverage Δ
src/invalidations.jl 87.04% <0.00%> (-3.12%) :arrow_down:
src/invalidation_and_inference.jl 0.00% <0.00%> (-100.00%) :arrow_down:
src/parcel_snoopi_deep.jl 87.74% <0.00%> (-1.84%) :arrow_down:
src/deep_demos.jl 57.89% <0.00%> (-1.08%) :arrow_down:
src/write.jl 91.30% <0.00%> (-1.01%) :arrow_down:
src/parcel_snoopl.jl 89.47% <0.00%> (-0.53%) :arrow_down:
SnoopCompileCore/src/snoopi_deep.jl 91.66% <0.00%> (-0.34%) :arrow_down:
src/parcel_snoopi.jl 93.83% <0.00%> (-0.31%) :arrow_down:
src/visualizations.jl 0.00% <0.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d18b722...71ab836. Read the comment docs.

timholy commented 2 years ago

Thanks! Do you have a reproducer for this? I generally attempt to capture the problem in a test before making the fix, to ensure no similar problems come up in the future.

charleskawczynski commented 2 years ago

I don't, but I can try to cook one up

timholy commented 2 years ago

Don't kill yourself for it, because it is indeed an obvious fix, but I thought I'd ask.

charleskawczynski commented 2 years ago

This is as far as I've gotten 😅:

using SnoopCompileCore

invalidations = @snoopr begin
    import BlockArrays
    import CUDA
end;

using SnoopCompile
trees = invalidation_trees(invalidations)

Also, this only "works" (results in an error) on Julia 1.6. I suppose inference on Julia 1.7 is better and we no longer hit this code path.

timholy commented 2 years ago

Thanks for the tip! I added a test that doesn't require those big packages. Here's how I captured the error: with SnoopCompile devved, I identified method that threw the error from the stacktrace and added this (Revise.jl makes this easy):

diff --git a/src/invalidations.jl b/src/invalidations.jl
index f8fd2db..37df189 100644
--- a/src/invalidations.jl
+++ b/src/invalidations.jl
@@ -173,7 +173,9 @@ end

 # We could use AbstractTrees here, but typically one is not interested in the full tree,
 # just the top method and the number of children it has
+const minvs = Ref{MethodInvalidations}()
 function Base.show(io::IO, methinvs::MethodInvalidations)
+    minvs[] = methinvs
     iscompact = get(io, :compact, false)::Bool

     print(io, methinvs.reason, " ")

This allowed me to extract (in the REPL0 the most recent input to the function via minvs = SnoopCompile.minvs[]. Then I was able to inspect it, identify which method definition in CUDA was responsible for the invalidation, and just mimic that.