Skip to content

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

Merged
NHDaly merged 16 commits into
JuliaDebug:masterfrom
NHDaly:nhd-snoopl
Nov 30, 2020
Merged

Add snoopl() to snoop time in LLVM optimization#135
NHDaly merged 16 commits into
JuliaDebug:masterfrom
NHDaly:nhd-snoopl

Conversation

@NHDaly

@NHDaly NHDaly commented Aug 20, 2020

Copy link
Copy Markdown
Collaborator

This uses the associated code added in JuliaLang/julia#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 added a commit to NHDaly/julia that referenced this pull request Oct 12, 2020
Adds these functions to toggle the output:
  - ccall(:jl_dump_emitted_mi_name, Nothing, (Ptr{Nothing},), e_io.handle)
  - ccall(:jl_dump_llvm_opt, Nothing, (Ptr{Nothing},), l_io.handle)

This will then be controlled from SnoopCompileCore, via JuliaDebug/SnoopCompile.jl#135.

This logs a CSV file with mappings from C function name to Julia
MethodInstance name, and a YAML file with timings statistics about the
LLVM module being optimized, including:
 - Total time to optimize the llvm module
 - The `optlevel` used for the llvm module
 - The name of each function in the module, before and after
 - Number of instructions for each function, before and after
 - Number of basic blocks for each function, before and after

Co-Authored-By: Sacha Verweij <sacha.verweij@alumni.stanford.edu>
Co-Authored-By: Valentin Churavy <v.churavy@gmail.com>
@NHDaly
NHDaly marked this pull request as ready for review October 26, 2020 16:46

@NHDaly NHDaly left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I believe this is now ready for review! :)

Comment thread src/SnoopCompile.jl Outdated
@NHDaly

NHDaly commented Oct 26, 2020

Copy link
Copy Markdown
Collaborator Author

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 😅

Comment thread src/SnoopCompile.jl Outdated
Comment thread src/parcel_snoopl.jl Outdated
@NHDaly
NHDaly force-pushed the nhd-snoopl branch 2 times, most recently from 2fba62f to c11f595 Compare October 29, 2020 15:58
Manually implement CSV reading, just like is done for `snoopc`.
@NHDaly

NHDaly commented Nov 30, 2020

Copy link
Copy Markdown
Collaborator Author

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

timholy commented Nov 30, 2020

Copy link
Copy Markdown
Member

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

NHDaly commented Nov 30, 2020

Copy link
Copy Markdown
Collaborator Author

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
NHDaly merged commit 341db1e into JuliaDebug:master Nov 30, 2020
@NHDaly
NHDaly deleted the nhd-snoopl branch November 30, 2020 16:14
@NHDaly

NHDaly commented Nov 30, 2020

Copy link
Copy Markdown
Collaborator Author

-- 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

timholy commented Nov 30, 2020

Copy link
Copy Markdown
Member

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

NHDaly commented Nov 30, 2020

Copy link
Copy Markdown
Collaborator Author

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

timholy commented Nov 30, 2020

Copy link
Copy Markdown
Member

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.

NHDaly added a commit that referenced this pull request Jul 12, 2021
I just noticed that `@snoopl` isn't in the documentation at all. I know it's not very widely used, but it'd be nice to refer to it here at least. :)

`@snoopl` was originally added in #135.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants