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.

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

Consider generate_precompile prior to module close #38951

Open
timholy opened this issue Dec 20, 2020 · 7 comments
Open

Consider generate_precompile prior to module close #38951

timholy opened this issue Dec 20, 2020 · 7 comments
Labels
compiler:precompilation Precompilation of modules

Comments

@timholy
Copy link
Member

timholy commented Dec 20, 2020

It's possible we should consider changing the strategy for the gen_precompile strategy; as noticed in #38906, not all statements are effectively precompilable. Here's a demo. First, if you precompile something that's well-and-truly compiled, it looks like this:

julia> mp = which(+, (Int, Int))
+(x::T, y::T) where T<:Union{Int128, Int16, Int32, Int64, Int8, UInt128, UInt16, UInt32, UInt64, UInt8} in Base at int.jl:87

julia> mp.specializations[1]
MethodInstance for +(::Int64, ::Int64)

julia> mp.specializations[1].cache
Core.CodeInstance(MethodInstance for +(::Int64, ::Int64), #undef, 0x0000000000000001, 0xffffffffffffffff, Int64, #undef, UInt8[0x0c, 0x03, 0x00, 0x00, 0x00, 0x00, 0x08, 0x08, 0x16, 0x88  …  0x21, 0x53, 0x52, 0x2b, 0x57, 0x00, 0xbf, 0x3d, 0x01, 0x01], true, true, Ptr{Nothing} @0x00007f110804b7e0, Ptr{Nothing} @0x00007f110804b7d0)

julia> @time precompile(+, (Int, Int))
  0.000010 seconds (2 allocations: 144 bytes)
true

Now let's try another method:

julia> m = which(Base.uncompressed_ir, (Method,))
uncompressed_ir(m::Method) in Base at reflection.jl:984

julia> m.specializations[1]
MethodInstance for Base.uncompressed_ir(::Method)

julia> m.specializations[1].cache
ERROR: UndefRefError: access to undefined reference
Stacktrace:
 [1] getproperty(x::Core.MethodInstance, f::Symbol)
   @ Base ./Base.jl:33
 [2] top-level scope
   @ REPL[7]:1

I'll show the time to precompile in a minute, but inspired by #32705 let's first peek at the method's roots:

julia> m.roots
6-element Vector{Any}:
 :(:source)
 :_uncompressed_ir
 :(:generator)
 "Method is @generated; try `code_lowered` instead."
 "Code for this Method is not available."
 :uncompressed_ir

julia> @time precompile(Base.uncompressed_ir, (Method,))
  0.011541 seconds (4.87 k allocations: 320.187 KiB, 99.91% compilation time)
true

Definitely not truly precompiled. And sure enough:

julia> m.roots
16-element Vector{Any}:
 :(:source)
 :_uncompressed_ir
 :(:generator)
 "Method is @generated; try `code_lowered` instead."
 "Code for this Method is not available."
 :uncompressed_ir
 typeof(isa)
 MethodInstance for copy(::Core.CodeInfo)
 :(:jl_uncompress_ir)
 :(:ccall)
 :($(QuoteNode(Ptr{Nothing} @0x0000000000000000)))
 :CodeInfo
 MethodInstance for error(::String)
 Symbol("Base.jl")
 isa (built-in function)
 Ptr{Nothing} @0x0000000000000000

The roots table expanded after compiling it, due to inlining its callees.

A way to solve this would to be to have our precompile generator emit statements before the final end of the module definition. That's a pretty big change in how we do this, of course; it would be worth discussing first whether #32705 can be fixed.

@KristofferC
Copy link
Member

A way to solve this would to be to have our precompile generator emit statements before the final end of the module definition.

What does this mean concretely? Is it enough to put them before the close of Base. Or does it have to be more fine grained?

@JeffBezanson
Copy link
Member

I'm not sure I understand why moving the precompile statements changes anything. Any insights?

@timholy
Copy link
Member Author

timholy commented Dec 21, 2020

I don't have the complete picture, but there have long been certain methods that just don't "take" when you try to precompile them. The phenotype is that you can add a precompile statement, and then the next @snoopi or @snoopi_deep in SnoopCompile shows you that inference ran all over again. I've assumed that it has something to do with #32705, but I'm not actually sure. For all the methods where it fails, it's definitely the case that running the code grows m.roots.

Here are some PRs that deal with the issue in a particularly focused way:

And here's a demo (you need MethodAnalysis and SnoopCompile installed):

# This script will create a new package, PrecompileTest, in your temporary directory
using Pkg, MethodAnalysis, SnoopCompile
pkgdir = joinpath(tempdir(), "PrecompileTest")
Pkg.generate(pkgdir)
open(joinpath(pkgdir, "src", "PrecompileTest.jl"), "w") do io
    write(io, """
    module PrecompileTest

    function precompile_me()
        # Pick a Dict type that Julia hasn't precompiled
        dict = Dict{Cmd,Float16}()
        key = `ls -a`
        dict[key] = 2
        return dict, sin(dict[key])
    end

    and_me() = exp(Float16(2))

    if ccall(:jl_generating_output, Cint, ()) == 1
        @assert precompile(precompile_me, ())
        @assert precompile(and_me, ())
    end

    end
    """
    )
end

# First, show that a regular Julia session lacks MethodInstances
@info "The following 3 lines should be `nothing`"
display(methodinstance(sin, (Float16,)))
display(methodinstance(exp, (Float16,)))
display(methodinstance(setindex!, (Dict{Cmd,Float16}, Float16, Cmd)))

push!(LOAD_PATH, dirname(pkgdir))
using PrecompileTest
# Check for backedges
@info "The next few lines should have MethodInstance content"
mi = methodinstance(sin, (Float16,))
display(mi)
display(mi.backedges)  # all is good with the world!
mi = methodinstance(exp, (Float16,))
display(mi)
isa(mi, Core.MethodInstance) && display(mi.backedges)  # the `isa` is to facilitate commenting out, see below
@warn "The next should too, but it doesn't"
mi = methodinstance(setindex!, (Dict{Cmd,Float16}, Float16, Cmd))
display(mi)            # uh oh

# Now run it in this session
@warn "Running `precompile_me` while snooping on inference --- this is bad because `setindex!` is really expensive to infer; it also seems to mess up others"
tinf = @snoopi PrecompileTest.precompile_me()
display(tinf)
@info "Running `and_me` while snooping on inference---all's great here!"
tinf = @snoopi PrecompileTest.and_me()
display(tinf)
@info "Try commenting out the `precompile(and_me, ())` line, you'll see it worked only because we'd precompiled it"

Here is the output:

julia> include("pctest.jl")
  Generating  project PrecompileTest:
    /tmp/PrecompileTest/Project.toml
    /tmp/PrecompileTest/src/PrecompileTest.jl
[ Info: The following 3 lines should be `nothing`
nothing
nothing
nothing
[ Info: Precompiling PrecompileTest [4a68bf2e-391a-4167-b5ef-fb7a905be883]
[ Info: The next few lines should have MethodInstance content
MethodInstance for sin(::Float16)
1-element Vector{Any}:
 MethodInstance for PrecompileTest.precompile_me()
MethodInstance for exp(::Float16)
1-element Vector{Any}:
 MethodInstance for PrecompileTest.and_me()
┌ Warning: The next should too, but it doesn't
└ @ Main ~/.julia/dev/pctest.jl:45
nothing
┌ Warning: Running `precompile_me` while snooping on inference --- this is bad because `setindex!` is really expensive to infer; it also seems to mess up others
└ @ Main ~/.julia/dev/pctest.jl:50
4-element Vector{Tuple{Float64, Core.MethodInstance}}:
 (0.0008871555328369141, MethodInstance for Base.ht_keyindex(::Dict{Cmd, Float16}, ::Cmd))
 (0.0017960071563720703, MethodInstance for Dict{Cmd, Float16}())
 (0.02426600456237793, MethodInstance for setindex!(::Dict{Cmd, Float16}, ::Int64, ::Cmd))
 (0.03497600555419922, MethodInstance for sin(::Float32))
[ Info: Running `and_me` while snooping on inference---all's great here!
Tuple{Float64, Core.MethodInstance}[]
[ Info: Try commenting out the `precompile(and_me, ())` line, you'll see it worked only because we'd precompiled it

precompile_me is eminently inferrable, and notice that it didn't need inference...but several of its callees did. This is always true for setindex! and Dicts if Julia hasn't itself compiled the methods, which is unfortunate because they are quite expensive. What I found in #38906 was that putting many methods in the generate_precompile.jl script was not enough, @snoopi showed that Revise still had to re-infer them.

For reference, here's currently what Revise has to infer (this is running on #38906, so those particular methods are now gone). I'm doing this demo from ~/.julia/dev/Example/src:

julia> Revise   # check that it's not loaded
ERROR: UndefVarError: Revise not defined

julia> using SnoopCompile

julia> @snoopi using Revise
2-element Vector{Tuple{Float64, Core.MethodInstance}}:
 (9.512901306152344e-5, MethodInstance for JuliaInterpreter.__init__())
 (0.01444697380065918, MethodInstance for setindex!(::Dict{Base.PkgId, Revise.PkgData}, ::Revise.PkgData, ::Base.PkgId))

julia> @snoopi begin
           # I've created a file, Example1.jl, with a small change to Example.jl
           cp("Example1.jl", "Example.jl"; force=true)
           sleep(0.01)   # for FileWatching
           revise()
       end
5-element Vector{Tuple{Float64, Core.MethodInstance}}:
 (0.00021195411682128906, MethodInstance for Revise.revise())
 (0.00179290771484375, MethodInstance for Dict{Tuple{Revise.PkgData, String}, Nothing}())
 (0.0025949478149414062, MethodInstance for sort!(::Vector{Tuple{Revise.PkgData, String}}, ::Int64, ::Int64, ::Base.Sort.MergeSortAlg, ::Base.Order.Lt{Base.Order.var"#1#3"{typeof(Revise.pkgfileless), typeof(identity)}}, ::Vector{Tuple{Revise.PkgData, String}}))
 (0.004344940185546875, MethodInstance for empty!(::Dict{Tuple{Revise.PkgData, String}, Nothing}))
 (0.008261919021606445, MethodInstance for copyto!(::Vector{Tuple{Revise.PkgData, String}}, ::Set{Tuple{Revise.PkgData, String}}))

That's pretty good, but it's like that only because of #38906 and because I and others have ensured that Revise and its whole stack are very well inferrable (check PRs to JuliaInterpreter, LoweredCodeUtils, and Revise over the last six months) to make it eminently precompilable---virtually all of the latency is codegen, as you can see from the fairly small numbers above. By contrast, a package like Makie is definitely not well precompilable, see MakieOrg/Makie.jl#792, and inference accounts for the majority of its TTFP. The flamegraphs there are not profile data but inference-time data.

@timholy
Copy link
Member Author

timholy commented Dec 22, 2020

What does this mean concretely? Is it enough to put them before the close of Base. Or does it have to be more fine grained?

Not sure, but my guess is that for precompile(f, (argtypes...)) you want to emit the statement in which(f, (argtypes...)).module before that module closes. Of course in some cases that's not possible, like push!(v::Vector{T}, obj::T) where T is defined in some stdlib. If it helps, SnoopCompile master branch has some code to walk the tree of inferred MethodInstances and find the first one where the module of the method knows about all the types.

@NHDaly
Copy link
Member

NHDaly commented Jan 11, 2021

@timholy - Do you think this phenomenon of method instances that don't serialize their precompilation results is the same thing that I observed, here?:
#28808 (comment)

@timholy
Copy link
Member Author

timholy commented Jan 12, 2021

I suspect it's the same. setindex!(::Dict, ...) methods are infamous for this kind of thing.

@StefanKarpinski
Copy link
Member

Still worth considering?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:precompilation Precompilation of modules
Projects
None yet
Development

No branches or pull requests

6 participants