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.

Sign up for GitHub

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

Parallelize Pkg.precompile #2018

Merged
merged 15 commits into from
Sep 15, 2020
Merged

Parallelize Pkg.precompile #2018

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
9937dea
add is_stdlib() lookup with string name
IanButterworth Sep 13, 2020
6fb919d
depth-first parallelization for Pkg.precompile
IanButterworth Sep 13, 2020
4708b59
remove unnecessary sort
IanButterworth Sep 13, 2020
5b0e5ac
limit threads via optional ENV var
IanButterworth Sep 13, 2020
7b98298
switch to Event triggers
IanButterworth Sep 13, 2020
4b2eef4
internalize is_stdlib_from_name as unsafe for general use
IanButterworth Sep 13, 2020
84697d7
switch to Base.Semafore
IanButterworth Sep 13, 2020
8f73605
switch to uuid-based dep checking
IanButterworth Sep 13, 2020
2b1379e
force comp if any dep was recompiled
IanButterworth Sep 14, 2020
10cbdc0
move to @sync, catch and handle errors, improved wait loop
IanButterworth Sep 14, 2020
a4bdd79
remove TODOs
IanButterworth Sep 14, 2020
52ef225
skip stale check if any dep was recompiled
IanButterworth Sep 14, 2020
ab03371
add comment on any_dep_recompiled
IanButterworth Sep 14, 2020
22c4687
clearer var name
IanButterworth Sep 15, 2020
dd0ad54
improved handling of error state for jobs queued before error
IanButterworth Sep 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 24 additions & 4 deletions src/API.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -895,14 +895,22 @@ end
precompile() = precompile(Context())
function precompile(ctx::Context)
printpkgstyle(ctx, :Precompiling, "project...")

man = Pkg.Types.read_manifest(ctx.env.manifest_file)
pkgids = [Base.PkgId(first(dep), last(dep).name) for dep in man if !Pkg.Operations.is_stdlib(first(dep))]
Copy link
Member

Choose a reason for hiding this comment

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

I know this filtering was here before, but why is it necessary to filter out stdlibs?

(tmp.9L2rmMdEXO) pkg> st
Status `/tmp/tmp.9L2rmMdEXO/Project.toml`
  [37e2e46d] LinearAlgebra

(tmp.9L2rmMdEXO) pkg> precompile
Precompiling project...

julia> using LinearAlgebra
[ Info: Precompiling LinearAlgebra [37e2e46d-f89d-539d-b4ee-838fcccc9c8e]

Copy link
Member Author

Choose a reason for hiding this comment

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

Aren't stdlib's always going to be precompiled already, and if you're dev-ing them they'd need to have their uuid removed, so wouldn't identify as stdlibs in that check?

Copy link
Member

Choose a reason for hiding this comment

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

Right, forgot to say that this is when you compile Julia without them in the sysimg. Perhaps we can instead filter based on if the package is already loaded? That should work for both regular packages and stdlibs. If it is a stdlib that is in the sysimg it doesn't need to precompile, and if it is a regular package that is already loaded in the session it is probably just precompiled from the using?

Copy link
Member Author

Choose a reason for hiding this comment

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

But what if they're loaded, and in need of recompiling? Perhaps the filter just isn't needed at all?

Copy link
Member

Choose a reason for hiding this comment

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

Yea I am not sure what happens if you try to precompile stdlibs that are loaded though? Since no precompiles files exist, will that spend time on precompiling them anyway? At least we can add the filter I suggested to the stdlibs.

Copy link
Member Author

Choose a reason for hiding this comment

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

#2021 updated with this now (the PkgId version)

Copy link
Member

Choose a reason for hiding this comment

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

I thought the stdlib check was for some kind of optimization (launching julia takes a few hundreds of ms even if you don't do anything). So, I think the correct predicate here is "is it in sysimage?" than "is it a stdlib?" since non-stdlib packages can be in sysimage and there is no point in calling compilecache for them. This also covers the exotic situation where some stdlibs are not in sysimage.

I think is_stdlib_and_loaded is better than nothing. But I feel it's a bit half-way solution if my assumption (the stdlib check was optimization) is correct.

Copy link
Member

Choose a reason for hiding this comment

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

Thats true, I didn't think about regular packages in the sysimg. But perhaps #2018 (comment) is a good enough approximation of that? It seems pretty strange to (i) load a dependency, (ii) update its version, (iii) pkg> precompile, (iv) restart Julia and expect everything to be precompiled?

Copy link
Member Author

Choose a reason for hiding this comment

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

Although #2021 is looking good, I do like the properness of in_sysimage. It explains exactly why we'd always want to skip. I'll prepare a PR

Copy link
Member

Choose a reason for hiding this comment

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

It seems pretty strange to (i) load a dependency, (ii) update its version, (iii) pkg> precompile, (iv) restart Julia and expect everything to be precompiled?

@fredrikekre Hmm... That was my expectation, actually. I generally expect pkg> $cmd and shell> jlpkg $cmd to be identical (when a project is not activated). Anyway, what do you think about #2021 + JuliaLang/julia#37652? I think in_sysimage is simple enough and nice to have.

pkg_dep_lists = [collect(keys(last(dep).deps)) for dep in man if !Pkg.Operations.is_stdlib(first(dep))]
filter!.(!Pkg.Operations.is_stdlib, pkg_dep_lists)

pkgids = [Base.PkgId(uuid, name) for (name, uuid) in ctx.env.project.deps if !is_stdlib(uuid)]
if ctx.env.pkg !== nothing && isfile( joinpath( dirname(ctx.env.project_file), "src", ctx.env.pkg.name * ".jl") )
push!(pkgids, Base.PkgId(ctx.env.pkg.uuid, ctx.env.pkg.name))
end
push!(pkg_dep_lists, collect(keys(ctx.env.project.deps)))
end

precomp_list = String[]
precomp_tasks = Task[]
IanButterworth marked this conversation as resolved.
Show resolved Hide resolved

# TODO: since we are a complete list, but not topologically sorted, handling of recursion will be completely at random
for pkg in pkgids
for (i, pkg) in pairs(pkgids)
IanButterworth marked this conversation as resolved.
Show resolved Hide resolved
paths = Base.find_all_in_cache_path(pkg)
sourcepath = Base.locate_package(pkg)
sourcepath === nothing && continue
Expand All @@ -917,9 +925,21 @@ function precompile(ctx::Context)
break
end
if stale
Base.compilecache(pkg, sourcepath)
t = @async begin
staticfloat marked this conversation as resolved.
Show resolved Hide resolved
if length(pkg_dep_lists[i]) > 0 # if pkg has deps
while length(precomp_list) == 0 || !all(map(x->in(x, precomp_list), pkg_dep_lists[i])) # if all deps not precomped
sleep(0.001)
end
fredrikekre marked this conversation as resolved.
Show resolved Hide resolved
end
Base.compilecache(pkg, sourcepath)
Copy link
Member

Choose a reason for hiding this comment

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

To limit parallelization, I suggest launching a million tasks like this, but then creating a Channel(num_tasks), then having each task put!() something into it just before this call to compilecache, then when it's finished, you take!() something back out. This will create, essentially, an N-parallel critical section, and allow N tasks to be running that section at once, while all others are blocked, waiting for the channel to free up space.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice. Ok

Copy link
Member

Choose a reason for hiding this comment

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

What happens if this throws?

Copy link

Choose a reason for hiding this comment

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

@staticfloat aren't you describing a (counting) Semaphore?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, pretty much.

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR now has a Semafore approach, and I tried out a channel-based approach here, which doesn't seem simpler master...ianshmean:ib/parallel_precomp_chanelbased

What should we move forward with?

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @tkf just to bring the conversation to a single thread

Copy link
Member

Choose a reason for hiding this comment

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

I'd actually implement precompile differently by using a recursive function that returns was_recompiled::Bool and wrapping the recursive calls with tasks. This way, we don't need to implement a future-like construct (i.e., was_processed + was_recompiled). The error handling would probably be more straightforward this way. Resource control is probably still easier with semaphore (unless we have an easy-to-use task pool and future in Base or stdlib) although I wish there were Base.acquire(f, semaphore).

But this is the kind of thing the trade-off is not completely clear until you have a concrete implementation. So, I think it's reasonable to defer this to future refactoring.

push!(precomp_list, pkg.name)
end
push!(precomp_tasks, t)
else
push!(precomp_list, pkg.name)
end
end
wait.(precomp_tasks)
nothing
end

Expand Down
1 change: 1 addition & 0 deletions src/Types.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,7 @@ function stdlibs()
return STDLIB[]
end
is_stdlib(uuid::UUID) = uuid in keys(stdlibs())
is_stdlib(name::String) = name in values(stdlibs())
IanButterworth marked this conversation as resolved.
Show resolved Hide resolved

Context!(kw_context::Vector{Pair{Symbol,Any}})::Context =
Context!(Context(); kw_context...)
Expand Down