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

Add snoopl() to snoop time in LLVM optimization #135

Closed NHDaly closed 3 years ago

NHDaly commented 4 years ago

This uses the associated code added in https://github.com/JuliaLang/julia/pull/37136 to get its timings.

Adds a macro to SnoopCompileCore, @snoopl, which collects timings for the LLVM optimization of compilation, and adds functions to display and understand the resulting timings.

Co-Authored-By: @Sacha0 @vchuravy

NHDaly commented 3 years ago

The julia 1.2 and 1.0 test failures seem to be related to a versions conflict between CSV, which adds DataFrames, and JLD, which is a test-only dependency:

25l25h   Testing SnoopCompile
 Resolving package versions...
ERROR: Unsatisfiable requirements detected for package HDF5 [f67ccb44]:
 HDF5 [f67ccb44] log:
 ├─possible versions are: [0.9.4-0.9.5, 0.10.0-0.10.4, 0.11.0-0.11.1, 0.12.0-0.12.5, 0.13.0-0.13.6] or uninstalled
 ├─restricted by compatibility requirements with JLD [4138dd39] to versions: [0.9.4-0.9.5, 0.10.0-0.10.4, 0.11.0-0.11.1, 0.12.0-0.12.5, 0.13.0-0.13.6]
 │ └─JLD [4138dd39] log:
 │   ├─possible versions are: [0.9.0-0.9.2, 0.10.0] or uninstalled
 │   ├─restricted to versions * by an explicit requirement, leaving only versions [0.9.0-0.9.2, 0.10.0]
 │   └─restricted by compatibility requirements with Compat [34da2185] to versions: [0.9.2, 0.10.0] or uninstalled, leaving only versions: [0.9.2, 0.10.0]
 │     └─Compat [34da2185] log:
 │       ├─possible versions are: [1.0.0-1.0.1, 1.1.0, 1.2.0, 1.3.0, 1.4.0, 1.5.0-1.5.1, 2.0.0, 2.1.0, 2.2.0-2.2.1, 3.0.0, 3.1.0, 3.2.0, 3.3.0-3.3.1, 3.4.0, 3.5.0, 3.6.0, 3.7.0, 3.8.0, 3.9.0-3.9.1, 3.10.0, 3.11.0, 3.12.0, 3.13.0, 3.14.0, 3.15.0, 3.16.0, 3.17.0, 3.18.0, 3.19.0, 3.20.0, 3.21.0, 3.22.0] or uninstalled
 │       └─restricted to versions 3.22.0 by an explicit requirement, leaving only versions 3.22.0
 ├─restricted by julia compatibility requirements to versions: [0.9.4-0.9.5, 0.10.0-0.10.4, 0.11.0-0.11.1, 0.12.0-0.12.5] or uninstalled, leaving only versions: [0.9.4-0.9.5, 0.10.0-0.10.4, 0.11.0-0.11.1, 0.12.0-0.12.5]
 ├─restricted by compatibility requirements with Compat [34da2185] to versions: [0.10.0-0.10.4, 0.11.0-0.11.1, 0.12.0-0.12.5, 0.13.0-0.13.6] or uninstalled, leaving only versions: [0.10.0-0.10.4, 0.11.0-0.11.1, 0.12.0-0.12.5]
 │ └─Compat [34da2185] log: see above
 └─restricted by compatibility requirements with Blosc [a74b3585] to versions: 0.13.1-0.13.6 or uninstalled — no versions left
   └─Blosc [a74b3585] log:
     ├─possible versions are: [0.5.1, 0.6.0, 0.7.0] or uninstalled
     ├─restricted by julia compatibility requirements to versions: [0.5.1, 0.6.0] or uninstalled
     └─restricted by compatibility requirements with Compat [34da2185] to versions: [0.6.0, 0.7.0] or uninstalled, leaving only versions: 0.6.0 or uninstalled
       └─Compat [34da2185] log: see above

I'm not exactly sure what one is supposed to do about this. I've tried removing the compat bounds, and it still fails on julia 1.2... So I don't really understand what is causing the above, nor how to fix it.


For julia nightly, it seems that CSV itself is broken on latest master, unfortunately:

testing test_utf8_with_BOM.csv
CSV.File: Error During Test at /Users/nathandaly/.julia/packages/CSV/MKemC/test/runtests.jl:15
  Got exception outside of a @test
  LoadError: MethodError: no method matching typesubtract(::Type{Union{Missing, String}}, ::Type{Int64})
  Closest candidates are:
    typesubtract(::Any, ::Any, ::Int64) at compiler/typeutils.jl:66
  Stacktrace:
    [1] ts(T::Type, S::Type)
      @ CSV ~/.julia/packages/CSV/MKemC/src/utils.jl:214
    [2] nonstandardtype(T::Type)
      @ CSV ~/.julia/packages/CSV/MKemC/src/utils.jl:217
    [3] (::CSV.var"#17#23")(T::Type)
      @ CSV ./none:0
    [4] iterate(::Base.Iterators.Filter{CSV.var"#17#23", Vector{Type}})
      @ Base.Iterators ./iterators.jl:451
    [5] iterate
      @ ./generator.jl:44 [inlined]
    [6] Header
      @ ~/.julia/packages/CSV/MKemC/src/header.jl:232 [inlined]
    [7] CSV.Rows(source::String; header::Int64, normalizenames::Bool, datarow::Int64, skipto::Nothing, footerskip::Int64, limit::Int64, transpose::Bool, comment::Nothing, use_mmap::Nothing, ignoreemptylines::Bool, select::Nothing, drop::Nothing, missingstrings::Vector{String}, missingstring::String, delim::Nothing, ignorerepeated::Bool, quotechar::Char, openquotechar::Nothing, closequotechar::Nothing, escapechar::Char, dateformat::Nothing, dateformats::Nothing, decimal::UInt8, truestrings::Vector{String}, falsestrings::Vector{String}, type::Nothing, types::Nothing, typemap::Dict{Type, Type}, categorical::Nothing, pool::Float64, lazystrings::Bool, strict::Bool, silencewarnings::Bool, debug::Bool, parsingdebug::Bool, reusebuffer::Bool, kw::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
      @ CSV ~/.julia/packages/CSV/MKemC/src/rows.jl:142
    [8] CSV.Rows(source::String)
      @ CSV ~/.julia/packages/CSV/MKemC/src/rows.jl:142
    [9] testfile(file::String, kwargs::NamedTuple{(), Tuple{}}, expected_sz::Tuple{Int64, Int64}, expected_sch::Type, testfunc::NamedTuple{(:col1, :col2, :col3), Tuple{Vector{Float64}, Vector{Float64}, Vector{Float64}}}; dir::String)
      @ Main ~/.julia/packages/CSV/MKemC/test/testfiles.jl:6

However, the tests pass locally when i run them manually by copy/pasting between various versions of julia 😅

NHDaly commented 3 years ago

Sooooo this PR was waiting on writing better documentation, but since we've sort of delayed that for the @snoopi_deep work, I'm going to merge this now, as well, so that it's all available for when your blog post goes live.

I'll fix the conflicts now, repush, and merge when tests are green!

timholy commented 3 years ago

Sounds good. IMO, better to have it merged and be using it than having it sit in a PR. I think the process of writing documentation is best done hand-in-hand with actually using it. I'm content to keep this work on master and delay releases until we are happy with the API.

NHDaly commented 3 years ago

Sounds good. IMO, better to have it merged and be using it than having it sit in a PR. I think the process of writing documentation is best done hand-in-hand with actually using it. I'm content to keep this work on master and delay releases until we are happy with the API.

Yeah, I completely agree. I'm sorry to have left these PRs languishing for so long! Bad behavior on my part. Thanks for your support!


I've also updated the docstrings. I'll merge when green. Thanks!

NHDaly commented 3 years ago

-- Oh, sorry, I didn't notice that the github default isn't squash on this repo for me yet. 😬 sorry! I'll try to catch that next time

timholy commented 3 years ago

No worries on either count. I wasn't in a major hurry with this one, though I am intrigued to start playing with it. I've been more focused on the inference step just because there's more we can do about it, either through avoidance of bad design (which can help with LLVM too) or through caching via precompilation (which currently doesn't help LLVM). Still, I'm coming to realize that sometimes the former will be more important than the latter, so it's great to have this merged.

NHDaly commented 3 years ago

Yeah, same here actually. We've been mostly looking at @snoopi_deep, too, I think largely just because it's such a nice interface, and LLVM_OPT can't provide the same granularity or causality. At some point it may stop being true that optimizing the former will continue helping with the latter, but we haven't hit that point yet, i think.

timholy commented 3 years ago

It's also really nice to have the actual MethodInstances in hand. It makes all the analysis much easier and reliable. In theory we should be able to reconstruct them by parsing, but it's clearly more error-prone than just getting them directly.