Skip to content

Commit

Permalink
[REPLCompletions] async tab/hint completion
Browse files Browse the repository at this point in the history
REPL utilizes inference for smarter completions, but since its inference
engine is independent from the native cache and issues like #52763 in
`REPLInterpreter`'s inference, sometimes delays can be noticeable during
tab/hint-completion, and REPL can even freeze in the worst cases.

To address this, the commit allows tab/hint-completion to run
asynchronously on a `:default` thread. This approach removes delays in
multi-threaded process and prevents REPL hang in extreme cases like #52763.
In detail, for tab-completion, completion task simply runs on a separate
thread since completion itself does not block anything. For
hint-completion however, considering that a stuttering display of hints
can be bothersome, when the completion task doesn't conclude within
0.01 sec, we cancel the hint-completion to keep the input in the REPL
smooth.

There are still some points left open for discussion:
- Is it better to allocate a separate thread specifically for REPL
  completion? I'm not sure if this is even possible though.
- Should we make this an optional feature, considering the concern some
  users might have about reduced thread resources?
- For situations where completion hangs or is slow, like in #52763,
  canceling the completion task to release resources would be ideal.
  However, my understanding is that there's no safe method to cancel a
  task, is it correct?
- At present, this commit `Threads.@spawn`s at a relatively high level
  in the completion pipeline. But it might be more effective to
  `Threads.@spawn` closer to the point where inference is actually
  invoked, such as in `complete_line` or `repl_eval_ex`.
  • Loading branch information
aviatesk committed Jan 10, 2024
1 parent 2afc20c commit c500eec
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 6 deletions.
19 changes: 16 additions & 3 deletions stdlib/REPL/src/LineEdit.jl
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,16 @@ function check_for_hint(s::MIState)
# Requires making space for them earlier in refresh_multi_line
return clear_hint(st)
end
completions, partial, should_complete = complete_line(st.p.complete, st, s.active_module)::Tuple{Vector{String},String,Bool}
# spawn a new task for hint completion to avoid blocking the main thread
# if the completion task doesn't finish quickly, abandon hinting and return early
completion_task = Threads.@spawn :default complete_line(st.p.complete, st, s.active_module)
done_quickly = false
completion_timer = Timer(0.01) do _
done_quickly = istaskdone(completion_task)
end
wait(completion_timer); close(completion_timer)
done_quickly || return clear_hint(st) # TODO kill `completion_task`?
completions, partial, should_complete = fetch(completion_task)::Tuple{Vector{String},String,Bool}
isempty(completions) && return clear_hint(st)
# Don't complete for single chars, given e.g. `x` completes to `xor`
if length(partial) > 1 && should_complete
Expand Down Expand Up @@ -2395,7 +2404,8 @@ end
# jump_spaces: if cursor is on a ' ', move it to the first non-' ' char on the right
# if `delete_trailing`, ignore trailing ' ' by deleting them
function edit_tab(s::MIState, jump_spaces::Bool=false, delete_trailing::Bool=jump_spaces)
tab_should_complete(s) && return complete_line(s)
# tab complete on a separate thread (if available) to avoid blocking the main thread
tab_should_complete(s) && return Threads.@spawn :default complete_line(s)
set_action!(s, :edit_insert_tab)
push_undo(s)
edit_insert_tab(buffer(s), jump_spaces, delete_trailing) || pop_undo(s)
Expand Down Expand Up @@ -2503,7 +2513,10 @@ AnyDict(
"^_" => (s::MIState,o...)->edit_undo!(s),
"\e_" => (s::MIState,o...)->edit_redo!(s),
# Show hints at what tab complete would do by default
"*" => (s::MIState,data,c::StringLike)->(edit_insert(s, c); check_for_hint(s) && refresh_line(s)),
"*" => function (s::MIState,data,c::StringLike)
edit_insert(s, c)
check_for_hint(s) && refresh_line(s)
end,
"^U" => (s::MIState,o...)->edit_kill_line_backwards(s),
"^K" => (s::MIState,o...)->edit_kill_line_forwards(s),
"^Y" => (s::MIState,o...)->edit_yank(s),
Expand Down
19 changes: 16 additions & 3 deletions stdlib/REPL/src/REPLCompletions.jl
Original file line number Diff line number Diff line change
Expand Up @@ -646,6 +646,10 @@ function resolve_toplevel_symbols!(src::Core.CodeInfo, mod::Module)
return src
end

# the lock to avoid concurrent inference from multiple completion tasks, which could happen
# if user types fast rapidly (otherwise end up with corrupt inference states)
const REPL_INFERENCE_LOCK = ReentrantLock()

# lower `ex` and run type inference on the resulting top-level expression
function repl_eval_ex(@nospecialize(ex), context_module::Module; limit_aggressive_inference::Bool=false)
if (isexpr(ex, :toplevel) || isexpr(ex, :tuple)) && !isempty(ex.args)
Expand All @@ -662,6 +666,17 @@ function repl_eval_ex(@nospecialize(ex), context_module::Module; limit_aggressiv
end
lwr isa Expr || return Const(lwr) # `ex` is literal
isexpr(lwr, :thunk) || return nothing # lowered to `Expr(:error, ...)` or similar
trylock(REPL_INFERENCE_LOCK) || return nothing
result = try
repl_infer_ex(lwr, context_module, limit_aggressive_inference)
finally
unlock(REPL_INFERENCE_LOCK)
end
result === Union{} && return nothing # for whatever reason, callers expect this as the Bottom and/or Top type instead
return result
end

function repl_infer_ex(lwr::Expr, context_module::Module, limit_aggressive_inference::Bool=false)
src = lwr.args[1]::Core.CodeInfo

# construct top-level `MethodInstance`
Expand All @@ -680,9 +695,7 @@ function repl_eval_ex(@nospecialize(ex), context_module::Module; limit_aggressiv
# potential invalidations of `Core.Compiler` methods.
Base.invoke_in_world(COMPLETION_WORLD[], CC.typeinf, interp, frame)

result = frame.result.result
result === Union{} && return nothing # for whatever reason, callers expect this as the Bottom and/or Top type instead
return result
return frame.result.result
end

# `COMPLETION_WORLD[]` will be initialized within `__init__`
Expand Down

0 comments on commit c500eec

Please sign in to comment.