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

WIP: fix #552: use invoke_in_world #556

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion src/legacy_loading.jl
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,8 @@ function parse_pkg_files(id::PkgId)
# To reduce compiler latency, use runtime dispatch for `queue_includes!`.
# `queue_includes!` requires compilation of the whole parsing/expression-splitting infrastructure,
# and it's better to wait to compile it until we actually need it.
Base.invokelatest(queue_includes!, pkgdata, id)
#worldage[] = Base.get_world_counter()
invoke_revisefunc(queue_includes!, pkgdata, id)
return pkgdata
end

Expand Down
2 changes: 1 addition & 1 deletion src/loading.jl
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ function parse_pkg_files(id::PkgId)
# To reduce compiler latency, use runtime dispatch for `queue_includes!`.
# `queue_includes!` requires compilation of the whole parsing/expression-splitting infrastructure,
# and it's better to wait to compile it until we actually need it.
Base.invokelatest(queue_includes!, pkgdata, id)
invoke_revisefunc(queue_includes!, pkgdata, id)
return pkgdata
end

Expand Down
4 changes: 3 additions & 1 deletion src/lowered.jl
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,8 @@ function methods_by_execution(mod::Module, ex::Expr; kwargs...)
return methodinfo, docexprs, frame
end

_lower(m::Module, ex, world::UInt) = ccall(:jl_expand_in_world, Any, (Any, Module, Cstring, Cint, Csize_t), ex, m, "none", 0, world)

"""
methods_by_execution!(recurse=JuliaInterpreter.Compiled(), methodinfo, docexprs, mod::Module, ex::Expr;
mode=:eval, disablebp=true, skip_include=mode!==:eval, always_rethrow=false)
Expand Down Expand Up @@ -175,7 +177,7 @@ The other keyword arguments are more straightforward:
function methods_by_execution!(@nospecialize(recurse), methodinfo, docexprs, mod::Module, ex::Expr;
mode::Symbol=:eval, disablebp::Bool=true, always_rethrow::Bool=false, kwargs...)
mode ∈ (:sigs, :eval, :evalmeth, :evalassign) || error("unsupported mode ", mode)
lwr = Meta.lower(mod, ex)
lwr = lower_in_reviseworld(mod, ex)
isa(lwr, Expr) || return nothing, nothing
if lwr.head === :error || lwr.head === :incomplete
error("lowering returned an error, ", lwr)
Expand Down
51 changes: 46 additions & 5 deletions src/packagedef.jl
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,15 @@ const silence_pkgs = Set{Symbol}()
const depsdir = joinpath(dirname(@__DIR__), "deps")
const silencefile = Ref(joinpath(depsdir, "silence.txt")) # Ref so that tests don't clobber

"""
Revise.worldage

The world age Revise was started in. Needed so that Revise doesn't delete methods
from under itself.
"""
const worldage = Ref{Union{Nothing,UInt}}(nothing)
#using CodeTracking: worldage

##
## The inputs are sets of expressions found in each file.
## Some of those expressions will generate methods which are identified via their signatures.
Expand Down Expand Up @@ -262,10 +271,13 @@ function delete_missing!(exs_sigs_old::ExprsSigs, exs_sigs_new)
@debug "DeleteMethod" _group="Action" time=time() deltainfo=(sig, MethodSummary(m))
# Delete the corresponding methods
for p in workers()
try # guard against serialization errors if the type isn't defined on the worker
remotecall(Core.eval, p, Main, :(delete_method_by_sig($sig)))
catch
end
#try # guard against serialization errors if the type isn't defined on the worker
future = remotecall(Core.eval, p, Main, :(delete_method_by_sig($sig)))
finalizer(future) do f
invoke_revisefunc(Distributed.finalize_ref, f)
end
#catch
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need to catch all errors here, or can we only catch certain ones? This bit me during debugging, when I tried to throw an error in the finalizer to get a stacktrace for debugging.

Copy link
Owner

Choose a reason for hiding this comment

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

Certainly it's fine to comment out while debugging, or to catch e; isa(e, SomeKindOfError) && throw(e) if SomeKindOfError deserves special handling. What kind of errors did you have in mind?

Off-topic, but since we talked about backtrace() in our other discussion, just checking, do you know about catch_backtrace()? It's sometimes quite useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was more wondering, what kind of errors this was initially supposed to catch, because catching all possible errors seems seems a bit dangerous to me, since we possibly ignore errors the user would like to know about. So I was more thinking of something along the lines of e isa ErrorThatsDistributedsFault || rethrow(), i.e. that the default would be to rethrow errors. Perhaps that's just not possible in this case though.

Off-topic, but since we talked about backtrace() in our other discussion, just checking, do you know about catch_backtrace()?

I do now! 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or can we at least log those errors with something like @warn?

Copy link
Owner

Choose a reason for hiding this comment

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

I see your point. This probably is the "lazy" approach, but doing better requires that we anticipate two things:

  • it seems likely that many errors might have already been caught by the main process, and so a report from a worker would be redundant. However, I suspect this was more of a concern in previous versions of Revise where there was more try/catch stuff; now, it seems likely that it wouldn't even get here if it fails in the main process.
  • workers may not all have the same code---for example, in some cases I've deliberately loaded Gtk only in the main process and had the workers be just computational engines. In such cases, some revise attempts will fail, but it would be wrong to send a warning to the user who would mentally think "I know, I did it on purpose."

As long as we can avoid pestering users incorrectly, I'm all in favor of pestering them when they deserve it 😄.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a way to check whether an error was thrown during regular "revising" or something else? If not, I would propose we just keep it as it is currently, but use either @debug or Revise's logging facilities to log the error, so people who want to know about these kinds of errors can still get them.

Copy link
Owner

@timholy timholy Oct 8, 2020

Choose a reason for hiding this comment

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

The pkgdata might be in revision_errors but the needed info doesn't propagate down here. I think logging it makes sense. EDIT: but it probably doesn't make it here if it triggers an error during regular revision, so you could just give it a whirl? As long as you can distinguish the second point above, perhaps via the exception type?

#end
end
Base.delete_method(m)
# Remove the entries from CodeTracking data
Expand Down Expand Up @@ -627,6 +639,7 @@ function handle_deletions(pkgdata, file)
topmod = first(keys(mexsold))
fileok = file_exists(filep)
mexsnew = fileok ? parse_source(filep, topmod) : ModuleExprsSigs(topmod)
worldage[] = Base.get_world_counter()
if mexsnew !== nothing
delete_missing!(mexsold, mexsnew)
end
Expand Down Expand Up @@ -720,6 +733,7 @@ function revise(; throw=false)

# Do all the deletion first. This ensures that a method that moved from one file to another
# won't get redefined first and deleted second.
@show worldage[] = Base.get_world_counter()
revision_errors = []
queue = sort!(collect(revision_queue); lt=pkgfileless)
finished = eltype(revision_queue)[]
Expand Down Expand Up @@ -752,6 +766,7 @@ function revise(; throw=false)
mode ∈ (:sigs, :eval, :evalmeth, :evalassign) || error("unsupported mode ", mode)
exsold = get(fi.modexsigs, mod, empty_exs_sigs)
for rex in keys(exsnew)
@show Base.get_world_counter()
sigs, includes = eval_rex(rex, exsold, mod; mode=mode)
if sigs !== nothing
exsnew[rex] = sigs
Expand Down Expand Up @@ -1174,11 +1189,34 @@ function maybe_set_prompt_color(color)
return nothing
end

if VERSION < v"1.6.0-DEV.1162"
const invoke_revisefunc = Base.invokelatest
const lower_in_reviseworld = Meta.lower
else
function invoke_revisefunc(f, args...; kwargs...)
#@show worldage[]
#@show Base.get_world_counter()
#Base.show_backtrace(stdout, backtrace()[1:4])
#println()
return Base.invoke_in_world(worldage[], f, args...; kwargs...)
end
function lower_in_reviseworld(m::Module, @nospecialize(ex))
#@show worldage[]
#@show Base.get_world_counter()
#Base.show_backtrace(stdout, backtrace()[1:1])
#println()
return ccall(:jl_expand_in_world, Any,
(Any, Ref{Module}, Cstring, Cint, Csize_t),
ex, m, "none", 0, worldage[],
)
end
end

# On Julia 1.5.0-DEV.282 and higher, we can just use an AST transformation
# This uses invokelatest not for reasons of world age but to ensure that the call is made at runtime.
# This allows `revise_first` to be compiled without compiling `revise` itself, and greatly
# reduces the overhead of using Revise.
revise_first(ex) = Expr(:toplevel, :(isempty($revision_queue) || Base.invokelatest($revise)), ex)
revise_first(ex) = Expr(:toplevel, :(isempty($revision_queue) || (#=worldage[] = Base.get_world_counter(); =#invoke_revisefunc($revise))), ex)

@noinline function run_backend(backend)
while true
Expand Down Expand Up @@ -1277,6 +1315,7 @@ function init_worker(p)
end

function __init__()
worldage[] = Base.get_world_counter()
run_on_worker = get(ENV, "JULIA_REVISE_WORKER_ONLY", "0")
if !(myid() == 1 || run_on_worker == "1")
return nothing
Expand Down Expand Up @@ -1328,6 +1367,8 @@ function __init__()
id = PkgId(nothing, "@REPL")
pkgdatas[id] = pkgdata = PkgData(id, nothing)
# Set the lookup callbacks
CodeTracking.method_lookup_callback[] = x -> (worldage[] = Base.get_world_counter(); invoke_revisefunc(get_def, x))
CodeTracking.expressions_callback[] = x -> (worldage[] = Base.get_world_counter(); invoke_revisefunc(get_expressions, x))
CodeTracking.method_lookup_callback[] = get_def
CodeTracking.expressions_callback[] = get_expressions

Expand Down
2 changes: 2 additions & 0 deletions src/pkgs.jl
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ function maybe_add_includes_to_pkgdata!(pkgdata::PkgData, file::AbstractString,
parse_source!(fi.modexsigs, fullfile, mod)
if eval_now
# Use runtime dispatch to reduce latency
#Base.invoke_in_world(worldage[], instantiate_sigs!, fi.modexsigs; mode=:eval)
Base.invokelatest(instantiate_sigs!, fi.modexsigs; mode=:eval)
end
end
Expand Down Expand Up @@ -237,6 +238,7 @@ function _add_require(sourcefile, modcaller, idmod, modname, expr)
end
end
if complex
#Base.invoke_in_world(worldage[], eval_require_now, pkgdata, fileidx, filekey, sourcefile, modcaller, expr)
Base.invokelatest(eval_require_now, pkgdata, fileidx, filekey, sourcefile, modcaller, expr)
end
finally
Expand Down
2 changes: 1 addition & 1 deletion src/utils.jl
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ This is used to make stacktraces obtained with Revise more similar to those obta
without Revise, while retaining one entry to reveal Revise's involvement.
"""
function trim_toplevel!(bt)
# return bt # uncomment this line if you're debugging Revise itself
return bt
n = itoplevel = length(bt)
for (i, t) in enumerate(bt)
sfs = StackTraces.lookup(t)
Expand Down