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

check if cachefile dependencies can be fulfilled #21492

Merged
merged 1 commit into from
Apr 24, 2017
Merged

check if cachefile dependencies can be fulfilled #21492

merged 1 commit into from
Apr 24, 2017

Conversation

vchuravy
Copy link
Member

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?

base/loading.jl Outdated

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member Author

vchuravy commented Apr 22, 2017

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
Copy link
Contributor

tkelman commented Apr 22, 2017

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.

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

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.

base/loading.jl Outdated
@@ -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}()
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

According to?

Copy link
Contributor

Choose a reason for hiding this comment

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

consistency with

modules = Dict{Symbol,UInt64}()

and

modules = Dict{Symbol,UInt64}()

@tkelman tkelman added the needs tests Unit tests are required for this change label Apr 22, 2017
@vchuravy vchuravy removed the needs tests Unit tests are required for this change label Apr 22, 2017
@vchuravy
Copy link
Member Author

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

test/compile.jl Outdated
""")
rm(FooBarT_file)
@test Base.stale_cachefile(FooBarT2_file, joinpath(dir2, "FooBarT2.ji"))
# Before #21492 this would fail: Base.require(:FooBarT2)
Copy link
Contributor

Choose a reason for hiding this comment

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

why don't we run it then?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@vchuravy
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

vtjnash commented Apr 23, 2017

Those timings look good.

@tkelman tkelman merged commit 36bb61a into master Apr 24, 2017
@tkelman tkelman deleted the vc/21266 branch April 24, 2017 04:28
@bjarthur
Copy link
Contributor

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
Copy link
Member Author

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

@bjarthur
Copy link
Contributor

i'm on the master branch of each.

@KristofferC
Copy link
Member

KristofferC commented Apr 24, 2017

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
Copy link
Contributor

i've checkout out e0c497b and everything works fine.

@fredrikekre
Copy link
Member

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
Copy link
Member Author

@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
Copy link
Contributor

tkelman commented Apr 24, 2017

x-ref #19108, possibly relevant?

vchuravy added a commit that referenced this pull request Apr 24, 2017
This works around `Base.Iterators` shadowing `Iterators`,
as noted in #21492.
if mod == :Main || mod == :Core || mod == :Base
continue
# Module is already loaded
elseif isdefined(Main, mod)
Copy link
Member

Choose a reason for hiding this comment

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

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

vtjnash added a commit that referenced this pull request Apr 26, 2017
tkelman pushed a commit that referenced this pull request May 3, 2017
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.

7 participants