-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
pidlock cache file precompilation #49052
pidlock cache file precompilation #49052
Conversation
cca8c44
to
26aeddb
Compare
15bc157
to
f3b7f5d
Compare
Was going to say this. |
Added
|
stdlib/FileWatching/src/pidfile.jl
Outdated
@@ -38,6 +42,7 @@ Optional keyword arguments: | |||
- `refresh`: Keeps a lock from becoming stale by updating the mtime every interval of time that passes. | |||
By default, this is set to `stale_age/2`, which is the recommended value. | |||
- `wait`: If true, block until we get the lock, if false, raise error if lock fails. | |||
- `wait_msg`: Provide a string for it to be `@info` logged when waiting for the lock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should accept a function to print however wanted, as showing a @info
log might be too opinionated?
c6c9f76
to
9cb7f12
Compare
I was hoping this would mean one could
where the environment contains packages bigger than Foo, for instance, but because |
Would it not make sense to have a |
I think that would require knowing which file you need to wait for. The path of the cachefile isn't determinable before spawning off the child process, AFAICT |
Oh, I see what you mean function cachefile_lock(f_first::Function, f_waited::Function, pkg::Base.PkgId, path::String)
pidfile = string(path, ".pidlock")
if trymkpidlock(f_first, pidfile) == false
mkpidlock(f_waited, pidfile; wait_msg = "Waiting for another process to precompile $pkg")
end
end |
No urgency but I'm going to wait for Jameson's review/opinion before working more on this |
This seems like a great direction! Should we schedule a call to review details? |
9cb7f12
to
4f3e9e3
Compare
@vtjnash I've tried to rewrite this to follow what we discussed |
4f3e9e3
to
7261dd5
Compare
8425af6
to
07ca9dc
Compare
5f85352
to
e3c0031
Compare
@vtjnash this seems to be in a working state now. For aligning
|
e287108
to
e3c0031
Compare
Bump @vtjnash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love the trymkpidlock
API of return Union{Bool,Any}
or the naming of PidlockedError
, but overall seems fine
@@ -1884,8 +1884,17 @@ function _require(pkg::PkgId, env=nothing) | |||
@goto load_from_cache | |||
end | |||
# spawn off a new incremental pre-compile task for recursive `require` calls | |||
cachefile = compilecache(pkg, path) | |||
if isa(cachefile, Exception) | |||
cachefile_or_module = maybe_cachefile_lock(pkg, path) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in JuliaLang/Pkg.jl#3458 we found it to be possible for a fork-bomb to occur.
That makes me wonder how this will handle accidental recursion. Will it now just hang since the parent obtains the pid-lock, then shells out, and the child hangs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could maybe even try to detect this specific deadlock scenario by calling getppid
https://linux.die.net/man/3/getppid
The other question is how robust is this against cancellation, what happens on a killall -9 julia
e3c0031
to
1893c65
Compare
1893c65
to
288f4c9
Compare
I'll add to NEWS later as this just manages non-interactive code load precompilation currently, needs a Pkg.precompile update |
This avoids multiple processes entering precompile cache file generation for the same package at the same time, allowing the first to enter and the others will wait to use the cache file that the first generated.
Terminal 1
Terminal 2 (started just after above)
Perhaps the 2nd process should inform the user that it's waiting for another process to finish.ImplementedMaking this specific to the package source was relatively straightforward. Making it specific to the resulting cache file path seems basically impossible because of the way the cache file name is generated.