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

Method invalidations #712

Closed
odow opened this issue May 10, 2021 · 5 comments
Closed

Method invalidations #712

odow opened this issue May 10, 2021 · 5 comments
Labels
client About our HTTP client

Comments

@odow
Copy link

odow commented May 10, 2021

HTTP.jl is a large cause of method invalidations in JuMP

julia> using SnoopCompileCore

julia> invalidations = @snoopr begin
       using HTTP
       end;

julia> using SnoopCompile

julia> trees = invalidation_trees(invalidations)
4-element Vector{SnoopCompile.MethodInvalidations}:
 inserting joinpath(uri::URIs.URI, parts::String...) in URIs at /Users/oscar/.julia/packages/URIs/o9DQG/src/URIs.jl:509 invalidated:
   mt_backedges: 1: signature Tuple{typeof(joinpath), Any, String} triggered MethodInstance for jointail(::Any, ::String) (0 children)
   1 mt_cache

 inserting write(iod::HTTP.DebugRequest.IODebug, a...) in HTTP.DebugRequest at /Users/oscar/.julia/dev/HTTP/src/IODebug.jl:30 invalidated:
   backedges: 1: superseding write(io::IO, x1, xs...) in Base at io.jl:636 with MethodInstance for write(::IO, ::Int64, ::Int64, ::Int64, ::Int64) (1 children)

 inserting readuntil(iod::HTTP.DebugRequest.IODebug, f) in HTTP.DebugRequest at /Users/oscar/.julia/dev/HTTP/src/IODebug.jl:58 invalidated:
   backedges: 1: superseding readuntil(io::IO, target::AbstractString; keep) in Base at io.jl:884 with MethodInstance for readuntil(::IO, ::String) (15 children)
              2: superseding readuntil(s::IO, delim::AbstractChar; keep) in Base at io.jl:771 with MethodInstance for readuntil(::IO, ::Char) (216 children)
   18 mt_cache

 inserting write(iod::HTTP.DebugRequest.IODebug, x::String) in HTTP.DebugRequest at /Users/oscar/.julia/dev/HTTP/src/IODebug.jl:38 invalidated:
   backedges: 1: superseding write(io::IO, s::Union{SubString{String}, String}) in Base at strings/io.jl:185 with MethodInstance for write(::IO, ::String) (261 children)


julia> length(invalidations)
1043

These seem fixable. I'll take a look in the next few days unless someone beats me to it.

x-ref: jump-dev/JuMP.jl#2273

@odow
Copy link
Author

odow commented May 11, 2021

@timholy I might need some advice here.

HTTP.jl defines these write methods:

HTTP.jl/src/IODebug.jl

Lines 30 to 40 in c882f2e

Base.write(iod::IODebug, a...) =
(logwrite(iod, :write, join(a));
write(iod.io, a...))
Base.write(iod::IODebug, a::Array) =
(logwrite(iod, :write, join(a));
write(iod.io, a))
Base.write(iod::IODebug, x::String) =
(logwrite(iod, :write, x);
write(iod.io, x))

which invalidate methods like
write(io::IO, s::Union{String,SubString{String}})
due to the REPL calling write with a non-concrete IO
https://github.com/JuliaLang/julia/blob/67186f43f73392bf1390eeacea929e81d29bdb05/stdlib/REPL/src/Terminals.jl#L102-L118

Is adding a function barrier a valid fix? (The following cuts the number of
invalidations in half.) Or do I need to add type parameters to the structs to
avoid the non-concrete type? (Related: if this is not valid, then I guess this
is not an appropriate way to test invalidations and I need to re-build a new
Julia?)

Before:

julia> using SnoopCompileCore

julia> invalidations = @snoopr using HTTP;

julia> length(invalidations)
1041

julia> using SnoopCompile

julia> trees = invalidation_trees(invalidations)
4-element Vector{SnoopCompile.MethodInvalidations}:
 inserting joinpath(uri::URIs.URI, parts::String...) in URIs at /Users/oscar/.julia/packages/URIs/o9DQG/src/URIs.jl:509 invalidated:
   mt_backedges: 1: signature Tuple{typeof(joinpath), Any, String} triggered MethodInstance for jointail(::Any, ::String) (0 children)
   1 mt_cache

 inserting write(iod::HTTP.DebugRequest.IODebug, a...) in HTTP.DebugRequest at /Users/oscar/.julia/dev/HTTP/src/IODebug.jl:30 invalidated:
   backedges: 1: superseding write(io::IO, x1, xs...) in Base at io.jl:636 with MethodInstance for write(::IO, ::Int64, ::Int64, ::Int64, ::Int64) (1 children)

 inserting readuntil(iod::HTTP.DebugRequest.IODebug, f) in HTTP.DebugRequest at /Users/oscar/.julia/dev/HTTP/src/IODebug.jl:58 invalidated:
   backedges: 1: superseding readuntil(io::IO, target::AbstractString; keep) in Base at io.jl:884 with MethodInstance for readuntil(::IO, ::String) (15 children)
              2: superseding readuntil(s::IO, delim::AbstractChar; keep) in Base at io.jl:771 with MethodInstance for readuntil(::IO, ::Char) (216 children)

 inserting write(iod::HTTP.DebugRequest.IODebug, x::String) in HTTP.DebugRequest at /Users/oscar/.julia/dev/HTTP/src/IODebug.jl:38 invalidated:
   backedges: 1: superseding write(io::IO, s::Union{SubString{String}, String}) in Base at strings/io.jl:185 with MethodInstance for write(::IO, ::String) (260 children)

After:

julia> import REPL.Terminals

julia> const T = Terminals
REPL.Terminals

julia> _write(io, key) = write(io, key)
_write (generic function with 1 method)

julia> T.cmove_up(t::T.UnixTerminal, n) = _write(t.out_stream, "$(T.CSI)$(n)A")

julia> T.cmove_down(t::T.UnixTerminal, n) = _write(t.out_stream, "$(T.CSI)$(n)B")

julia> T.cmove_right(t::T.UnixTerminal, n) = _write(t.out_stream, "$(T.CSI)$(n)C")

julia> T.cmove_left(t::T.UnixTerminal, n) = _write(t.out_stream, "$(T.CSI)$(n)D")

julia> using SnoopCompileCore

julia> invalidations = @snoopr using HTTP;

julia> length(invalidations)
551

julia> using SnoopCompile

julia> trees = invalidation_trees(invalidations)
4-element Vector{SnoopCompile.MethodInvalidations}:
 inserting joinpath(uri::URIs.URI, parts::String...) in URIs at /Users/oscar/.julia/packages/URIs/o9DQG/src/URIs.jl:509 invalidated:
   mt_backedges: 1: signature Tuple{typeof(joinpath), Any, String} triggered MethodInstance for jointail(::Any, ::String) (0 children)
   1 mt_cache

 inserting write(iod::HTTP.DebugRequest.IODebug, a...) in HTTP.DebugRequest at /Users/oscar/.julia/dev/HTTP/src/IODebug.jl:30 invalidated:
   backedges: 1: superseding write(io::IO, x1, xs...) in Base at io.jl:636 with MethodInstance for write(::IO, ::Int64, ::Int64, ::Int64, ::Int64) (1 children)

 inserting write(iod::HTTP.DebugRequest.IODebug, x::String) in HTTP.DebugRequest at /Users/oscar/.julia/dev/HTTP/src/IODebug.jl:38 invalidated:
   backedges: 1: superseding write(io::IO, s::Union{SubString{String}, String}) in Base at strings/io.jl:185 with MethodInstance for write(::IO, ::String) (15 children)

 inserting readuntil(iod::HTTP.DebugRequest.IODebug, f) in HTTP.DebugRequest at /Users/oscar/.julia/dev/HTTP/src/IODebug.jl:58 invalidated:
   backedges: 1: superseding readuntil(io::IO, target::AbstractString; keep) in Base at io.jl:884 with MethodInstance for readuntil(::IO, ::String) (15 children)
              2: superseding readuntil(s::IO, delim::AbstractChar; keep) in Base at io.jl:771 with MethodInstance for readuntil(::IO, ::Char) (216 children)

@fredrikekre
Copy link
Member

Can probably remove this debug code and make it opt in when debugging or something.

@timholy
Copy link
Contributor

timholy commented Sep 30, 2021

Sorry I didn't notice this thread earlier. An inferrable function barrier won't change anything, because the chain will be unbroken via the backedges. A barrier that uses runtime dispatch will change something; you can use Base.invokelatest as a tool to achieve this.

One thing to point out: by default, SnoopCompile documents all method invalidations. As someone who went around just fixing Base + stdlibs to make them more robust, to me this seemed fine. However, if your goal is to make life better for a specific package (e.g., JuMP), then it's quite likely that not all invalidations are relevant. Historically the way you figured this out was to browse the invalidation trees and see if methods in your package were invalidated (which, of course, can only happen if they've already been precompiled). In the 2.8.0 release, SnoopCompile gained new functionality, https://timholy.github.io/SnoopCompile.jl/stable/reference/#SnoopCompile.precompile_blockers, which might be handy.

Be warned, though, that if HTTP is one of JuMP's dependencies (is it?), it's a little complex to wrap your head around, and I'm not fully confident it's reporting everything it should. The basic issue is this: when you precompile a package, it starts Julia, loads the dependencies, loads the package, executes any precompile directives or workload, and then caches the type-inferred methods to the *.ji file. If HTTP is a dependency of JuMP, then any invalidations in Base + stdlibs will happen prior to loading JuMP itself; if JuMP depends on that Base/stdlib functionality, it will recompile those methods as it compiles code in JuMP. Now imagine what happens when you load the package: JuMP depends on post-invalidation recompiled methods in Base/stdlibs, which don't exist in a fresh session. Boom, this blocks effective precompilation. But it can be quite hard to detect this without tools like precompile_blockers. However, because of complexities of loading order etc I have to say I'm not 100% confident that precompile_blockers always does the right thing; be on the lookout for weird results and please report them if you find them.

It's far more straightforward if HTTP is not a dependency of JuMP, but something that frequently gets loaded after loading JuMP and running some of its compiled code. In that case, I have much more confidence that precompile_blockers will do the right thing.

The most direct result of HTTP invalidating REPL methods would be making the REPL itself feel transiently sluggish. I think I am observing this, but it doesn't look like a huge effect.

@odow
Copy link
Author

odow commented Sep 30, 2021

Oops. I forgot about this. I switched the (transitive) HTTP dependency in JuMP to a test-dependency, which is why I never followed up. (We are using JSONSchema.jl to valid models JuMP writes to disk. But it's easier to ask users to load JSONSchema themselves rather than run JuMP.validate.)

Thanks for the explanation. I'll take a look at precompile_blockers.

@quinnj
Copy link
Member

quinnj commented May 25, 2022

Will be fixed via #826

@quinnj quinnj closed this as completed May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client About our HTTP client
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants