check if cachefile dependencies can be fulfilled #21492

vchuravy · 2017-04-22T07:28:14Z

Fixes #21266. I was not able to reproduce the original issue,
because the dependency chain changed and DataArrays also
depends on SpecialFunctions.

We are writing transitive dependencies to the .ji file,
because at the moment of loading the .ji files there is no
difference between transitive dependencies or direct dependencies.
Just a list of preconditions, that need to be fullfilled.

Instead of changing that fairly complex piece of logic during
feature freeze I decided to try a less disruptive approach
and check in stale_cachefile, if all dependencies are
available.

Any suggestions for tests?

vchuravy · 2017-04-22T07:28:58Z

base/loading.jl

+
+        # Check if transitive dependencies can be fullfilled
+        for mod in keys(required_modules)
+            isdefined(Main, mod) && continue


It might be better to check here for :Core, :Base, ....

why would staleness depend on the in memory state of Main?

I am checking if the module is already loaded here, since if it is read_verify_mod_list from dump.c, won't fail. Just added a commit to be more explicit (it safes a few calls to find_in_node_path)

vchuravy · 2017-04-22T07:42:08Z

A different solution that would probably work, but is imho tricky to implement: Ensure a consistent ordering in the module dependency list, so that direct dependencies are loaded first. But at the point where that list is written we have no information about direct or indirect dependencies.

tkelman · 2017-04-22T15:55:53Z

for testing, we should be able to create a stack of 3 placeholder modules. Precompile the top one, then remove the bottom one and change the middle to not depend on it any more. That should reproduce the failure on master I believe.

vtjnash

I hate to slow it down this much (I've seen find_in_node_path can be a hot spot), since it'll probably cause a measurable performance regression in #15048). But this looks valid.

musm · 2017-04-22T21:46:45Z

base/loading.jl

@@ -699,7 +699,16 @@ function parse_cache_header(f::IO)
        push!(files, (String(read(f, n)), ntoh(read(f, Float64))))
    end
    @assert totbytes == 4 "header of cache file appears to be corrupt"
-    return modules, files
+    # read the list of modules that are required to be present during loading
+    required_modules = Dict{Symbol, UInt64}()


nitpick :P but no space between {Symbol,UInt64}

According to?

consistency with

julia/base/loading.jl

Line 683 in 70fdbe8

modules = Dict{Symbol,UInt64}()

and

julia/base/loading.jl

Line 683 in 70fdbe8

modules = Dict{Symbol,UInt64}()

vchuravy · 2017-04-22T23:54:56Z

Added a test. (Developed against master with Base.require)

tkelman · 2017-04-22T23:56:26Z

test/compile.jl

+          """)
+    rm(FooBarT_file)
+    @test Base.stale_cachefile(FooBarT2_file, joinpath(dir2, "FooBarT2.ji"))
+    # Before #21492 this would fail: Base.require(:FooBarT2)


why don't we run it then?

My logic was that if the line above fails it should also fail (and we don't quite have a @test_nothrow), but you are right it shouldn't hurt also doing it here.

we could test that it returns === nothing if that's what it's supposed to do

vchuravy · 2017-04-23T03:00:46Z

I addressed all comments, there was one intermediate travis failure that is backed here https://gist.github.com/vchuravy/aa4a715b99ef33435892bfd934bcc577, but it seems unrelated.

vchuravy · 2017-04-23T04:14:07Z

with regard to performance (all tests done on a clean PKGDIR and after Pkg.init(); Pkg.add("DataFrames"))

This branch

# Precompilation run
JULIA_PKGDIR=${HOME}/jl21266 time ./julia -e "using DataFrames"
21.93user 5.97system 0:21.54elapsed 129%CPU (0avgtext+0avgdata 271452maxresident)k
0inputs+6000outputs (0major+160067minor)pagefaults 0swaps

# After precompilation
JULIA_PKGDIR=${HOME}/jl21266 time ./julia -e "using DataFrames"
1.47user 0.74system 0:01.42elapsed 155%CPU (0avgtext+0avgdata 211916maxresident)k
0inputs+0outputs (0major+11971minor)pagefaults 0swaps

Current master

JULIA_PKGDIR=${HOME}/jl21266 time ./julia -e "using DataFrames"
21.55user 6.02system 0:21.14elapsed 130%CPU (0avgtext+0avgdata 271000maxresident)k
0inputs+6016outputs (0major+160987minor)pagefaults 0swaps

 JULIA_PKGDIR=${HOME}/jl21266 time ./julia -e "using DataFrames"
1.49user 0.76system 0:01.48elapsed 151%CPU (0avgtext+0avgdata 214772maxresident)k
0inputs+0outputs (0major+12437minor)pagefaults 0swaps

@vtjnash is there a better way to check the performance impact? Right now it seems imperceptible, but it might be worse if the JULIA_PKGDIR is on a remote filesystem.

vtjnash · 2017-04-23T13:23:24Z

Those timings look good.

bjarthur · 2017-04-24T12:50:26Z

this PR breaks Gadfly.jl's ability to pre-compile:

julia> using Gadfly
INFO: Recompiling stale cache file /Users/arthurb/.julia/lib/v0.6/Gadfly.ji for module Gadfly.
WARNING: The call to compilecache failed to create a usable precompiled cache file for module Compose. Got:
WARNING: Module Iterators uuid did not match cache file.
WARNING: eval from module Main to Gadfly:    
Expr(:call, Expr(:., :Base, :include_from_node1)::Any, "/Users/arthurb/.julia/v0.6/Compose/src/Compose.jl")::Any
  ** incremental compilation may be broken for this module **

WARNING: imported binding for Iterators overwritten in module Main

vchuravy · 2017-04-24T12:56:18Z

Thanks for letting me know I am looking into it. What version of Compose/Gadfly are you using?

bjarthur · 2017-04-24T12:58:12Z

i'm on the master branch of each.

KristofferC · 2017-04-24T13:17:26Z

I have also gotten the WARNING: Module Iterators uuid did not match cache file. recently and that was before this commit. Are you sure this is the commit that broke it? (To solve it I had to delete Iterators.ji in .julia/v0.6/lib/ and retry a few times.

bjarthur · 2017-04-24T13:18:44Z

i've checkout out e0c497b and everything works fine.

fredrikekre · 2017-04-24T13:57:47Z

I got the same error just now, and as for @bjarthur everything works if i checkout e0c497b . I tried removing ~/.julia/lib/v0.6/ but I get the same failure.
Found this from using PGFPlots but (for me) it boils down to the following error message from using AxisArrays

julia> using AxisArrays
INFO: Recompiling stale cache file /home/fredrik/.julia/lib/v0.6/AxisArrays.ji for module AxisArrays.
WARNING: The call to compilecache failed to create a usable precompiled cache file for module AxisArrays. Got:
WARNING: Module Iterators uuid did not match cache file.
ERROR: LoadError: Declaring __precompile__(true) is only allowed in module files being imported.
Stacktrace:
 [1] __precompile__(::Bool) at ./loading.jl:318
 [2] __precompile__() at ./loading.jl:314
 [3] include_from_node1(::String) at ./loading.jl:539
 [4] eval(::Module, ::Any) at ./boot.jl:235
 [5] require(::Symbol) at ./loading.jl:453
while loading /home/fredrik/.julia/v0.6/AxisArrays/src/AxisArrays.jl, in expression starting on line 1

vchuravy · 2017-04-24T13:59:08Z

@vtjnash I might need your help here

In a fresh Julia session

julia> Base.module_uuid(Main.Iterators)
0x000003c21dc6b0cb

julia> using Iterators
WARNING: imported binding for Iterators overwritten in module Main

julia> Base.module_uuid(Main.Iterators)
0x0000057e0e7d6f6b

julia> Base.parse_cache_header(joinpath(first(Base.LOAD_CACHE_PATH), "Compose.ji"))
...,  :Iterators=>0x0000057e0e7d6f6b, ...

But when we are restoring the module in src/dump.c read_verify_mod_list, we first check if the binding is already defined in Main and so we do not load the Iterators module provided by the package and then have an uuid conflict with Base.iterators. This is a issue independent of the PR above.

tkelman · 2017-04-24T14:02:22Z

x-ref #19108, possibly relevant?

This works around `Base.Iterators` shadowing `Iterators`, as noted in #21492.

vtjnash · 2017-04-24T23:47:02Z

base/loading.jl

+            if mod == :Main || mod == :Core || mod == :Base
+                continue
+            # Module is already loaded
+            elseif isdefined(Main, mod)


needs a check for isbindingresolved(Main, mod) first, to avoid causing isdefined to resolve the binding

Fix wrong use of `isdefined` #21492

(cherry picked from commit 36bb61a)

vchuravy commented Apr 22, 2017

View reviewed changes

vchuravy requested review from vtjnash and tkelman April 22, 2017 07:29

vchuravy force-pushed the vc/21266 branch from d7bc11c to 70fdbe8 Compare April 22, 2017 14:15

tkelman added the backport pending 0.5 label Apr 22, 2017

vtjnash approved these changes Apr 22, 2017

View reviewed changes

musm reviewed Apr 22, 2017

View reviewed changes

tkelman added the needs tests Unit tests are required for this change label Apr 22, 2017

vchuravy force-pushed the vc/21266 branch from 70fdbe8 to da2ced6 Compare April 22, 2017 23:52

vchuravy removed the needs tests Unit tests are required for this change label Apr 22, 2017

tkelman reviewed Apr 22, 2017

View reviewed changes

vchuravy force-pushed the vc/21266 branch from da2ced6 to 9800437 Compare April 23, 2017 00:12

check if cachefile dependencies can be fulfilled

c831fa0

vchuravy force-pushed the vc/21266 branch from 9800437 to c831fa0 Compare April 23, 2017 02:58

tkelman merged commit 36bb61a into master Apr 24, 2017

tkelman deleted the vc/21266 branch April 24, 2017 04:28

vchuravy added a commit that referenced this pull request Apr 24, 2017

force a require if binding with different uuid is present.

7015525

This works around `Base.Iterators` shadowing `Iterators`, as noted in #21492.

vchuravy mentioned this pull request Apr 24, 2017

force a require if binding with different uuid is present. #21537

Closed

vtjnash reviewed Apr 24, 2017

View reviewed changes

fredrikekre mentioned this pull request Apr 25, 2017

Fix wrong use of isdefined #21492 #21539

Merged

vtjnash added a commit that referenced this pull request Apr 26, 2017

Merge pull request #21539 from JuliaLang/vc/fix21492

98414b9

Fix wrong use of `isdefined` #21492

tkelman pushed a commit that referenced this pull request May 3, 2017

check if cachefile dependencies can be fulfilled (#21492)

88ab843

(cherry picked from commit 36bb61a)

tkelman removed the backport pending 0.5 label May 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

check if cachefile dependencies can be fulfilled #21492

check if cachefile dependencies can be fulfilled #21492

vchuravy commented Apr 22, 2017

vchuravy Apr 22, 2017

tkelman Apr 22, 2017

vchuravy Apr 22, 2017

vchuravy commented Apr 22, 2017 •

edited

Loading

tkelman commented Apr 22, 2017

vtjnash left a comment

musm Apr 22, 2017

KristofferC Apr 22, 2017

musm Apr 22, 2017

vchuravy commented Apr 22, 2017

tkelman Apr 22, 2017

vchuravy Apr 23, 2017

tkelman Apr 23, 2017

vchuravy commented Apr 23, 2017

vchuravy commented Apr 23, 2017

vtjnash commented Apr 23, 2017

bjarthur commented Apr 24, 2017

vchuravy commented Apr 24, 2017

bjarthur commented Apr 24, 2017

KristofferC commented Apr 24, 2017 •

edited

Loading

bjarthur commented Apr 24, 2017

fredrikekre commented Apr 24, 2017

vchuravy commented Apr 24, 2017

tkelman commented Apr 24, 2017

vtjnash Apr 24, 2017

check if cachefile dependencies can be fulfilled #21492

check if cachefile dependencies can be fulfilled #21492

Conversation

vchuravy commented Apr 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vchuravy commented Apr 22, 2017 • edited Loading

tkelman commented Apr 22, 2017

vtjnash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vchuravy commented Apr 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vchuravy commented Apr 23, 2017

vchuravy commented Apr 23, 2017

This branch

Current master

vtjnash commented Apr 23, 2017

bjarthur commented Apr 24, 2017

vchuravy commented Apr 24, 2017

bjarthur commented Apr 24, 2017

KristofferC commented Apr 24, 2017 • edited Loading

bjarthur commented Apr 24, 2017

fredrikekre commented Apr 24, 2017

vchuravy commented Apr 24, 2017

tkelman commented Apr 24, 2017

Choose a reason for hiding this comment

vchuravy commented Apr 22, 2017 •

edited

Loading

KristofferC commented Apr 24, 2017 •

edited

Loading