From c2fb5b03e260e97cd66e76549acf210054453368 Mon Sep 17 00:00:00 2001 From: Tim Holy Date: Thu, 31 Dec 2020 12:34:20 -0600 Subject: [PATCH 1/3] Improve precompilability with noninferrable filename After #259, the result of `filename(q)` is not inferrable. This uses "the kernel trick" to provide a single point where the result must be inferred, and an `invokelatest` to prevent the abstract signature from being inferred. It then precompiles the `::String` variant. Consequently, with respect to latency this should get us back to where we were before #259. --- src/loadsave.jl | 10 +++++++--- src/precompile.jl | 6 ++++++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/loadsave.jl b/src/loadsave.jl index a5dd85dc..63730ed8 100644 --- a/src/loadsave.jl +++ b/src/loadsave.jl @@ -182,14 +182,18 @@ end # Handlers for formatted files/streams for fn in (:load, :loadstreaming, :metadata) + fn_func_name = Symbol(fn, "_filename") gen2_func_name = Symbol("fileio_", fn) @eval function $fn(@nospecialize(q::Formatted), @nospecialize(args...); @nospecialize(options...)) + Base.invokelatest($fn_func_name, q, filename(q), args...; options...) + end + @eval function $fn_func_name(@nospecialize(q::Formatted), filename, @nospecialize(args...); @nospecialize(options...)) if unknown(q) - isfile(filename(q)) || open(filename(q)) # force systemerror + isfile(filename) || open(filename) # force systemerror throw(UnknownFormat(q)) end if q isa File - !isfile(filename(q)) && throw(ArgumentError("No file exists at given path: $(filename(q))")) + !isfile(filename) && throw(ArgumentError("No file exists at given path: $(filename)")) end libraries = applicable_loaders(q) failures = Any[] @@ -207,7 +211,7 @@ for fn in (:load, :loadstreaming, :metadata) push!(failures, (e, q)) end end - handle_exceptions(failures, "loading $(repr(filename(q)))") + handle_exceptions(failures, "loading $(repr(filename))") end end diff --git a/src/precompile.jl b/src/precompile.jl index 3a74a69b..39392b62 100644 --- a/src/precompile.jl +++ b/src/precompile.jl @@ -9,6 +9,12 @@ function _precompile_() @assert precompile(Tuple{typeof(load),File}) @assert precompile(Tuple{typeof(load),Formatted}) @assert precompile(Tuple{typeof(load),String}) + @assert precompile(Tuple{typeof(FileIO.load_filename),Formatted,String}) + if isdefined(Base, :bodyfunction) + fbody = Base.bodyfunction(which(FileIO.load_filename, (Formatted, String))) + @assert precompile(fbody, (Any, typeof(FileIO.load_filename), Formatted, String)) + @assert precompile(fbody, (Any, typeof(FileIO.load_filename), Formatted, String, Vararg{Any,100})) + end @assert precompile(Tuple{typeof(query),String}) @assert precompile(Tuple{typeof(query),IOStream}) From 35d62f6f9490b177108bc58477eaf6cbd5c1fbcb Mon Sep 17 00:00:00 2001 From: Tim Holy Date: Thu, 31 Dec 2020 12:36:36 -0600 Subject: [PATCH 2/3] Detach `applicable_loaders` from its error path For reasons I don't fully understand, either the string interpolation or the error messes up precompilation of `applicable_loaders`. This splits it out into a separate function and calls it via `invokelatest` to prevent it from blocking precompilation. --- src/loadsave.jl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/loadsave.jl b/src/loadsave.jl index 63730ed8..2e480107 100644 --- a/src/loadsave.jl +++ b/src/loadsave.jl @@ -34,6 +34,7 @@ function checked_import(pkg::Symbol) end end +applicable_error(applicable, sym) = error("No $applicable found for $sym") for (applicable_, add_, dict_) in ( (:applicable_loaders, :add_loader, :sym2loader), @@ -44,7 +45,7 @@ for (applicable_, add_, dict_) in ( if haskey($dict_, sym) return $dict_[sym] end - error("No $($applicable_) found for $(sym)") + Base.invokelatest(applicable_error, $applicable_, sym) end function $add_(@nospecialize(fmt::Type{<:DataFormat}), pkg::Symbol) sym = formatname(fmt) From 6f009f01f6650abec4da19a5b7b8379e417ed493 Mon Sep 17 00:00:00 2001 From: Tim Holy Date: Thu, 31 Dec 2020 12:38:57 -0600 Subject: [PATCH 3/3] Eliminate the offer to interactively add packages I haven't seen this warning come up in a long time, so I don't think this code is active. It adds significant latency due to the time needed to infer the Pkg calls, so let's ditch it. xref #226. --- src/error_handling.jl | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/src/error_handling.jl b/src/error_handling.jl index 31c0c620..572e8676 100644 --- a/src/error_handling.jl +++ b/src/error_handling.jl @@ -83,20 +83,5 @@ handle_error(e, q) = throw(e) function handle_error(e::NotInstalledError, q) println("Library \"", e.library, "\" is not installed but is recommended as a library to load format: \"", file_extension(q), "\"") - !isinteractive() && rethrow(e) # if we're not in interactive mode just throw - while true - println("Should we install \"", e.library, "\" for you? (y/n):") - input = lowercase(chomp(strip(readline(stdin)))) - if input == "y" - @info(string("Start installing ", e.library, "...")) - Pkg.add(string(e.library)) - return false # don't continue - elseif input == "n" - @info(string("Not installing ", e.library)) - return true # User does not install, continue going through errors. - else - println("$input is not a valid choice. Try typing y or n") - end - end - true # User does not install, continue going through errors. + rethrow(e) end