[clangd-dev] LSP Cancellations - making it universal and measurable?

[Sam McCall via clangd-dev]

Hey Ilya and Kadir,

Was trying to understand how much we (can) win from cancellation, and
what's involved in instrumenting a LSP method for cancellation.

Have a couple of proposals, wanted to discuss first them rather than
sending a patch because they're separate but interact.

1) task should record the time of requested cancellation. For analysis,
there are 3 interesting timestamps - task start, (optional) cancellation,
and end. The creator of the task currently can get start and end (via
context cleanup), but not cancellation time. This allows us to measure how
much we can improve cancellation (by bailing out "lame duck" tasks earlier).
Implementation is easy, just change the atomic<bool> to
atomic<steady_clock::rep>

2) we should support cancellation of any method, even if early bailout
isn't yet implemented.
Main benefit: we can then measure the lame duck time for all methods (as
above), and know where to implement early bailout. Side benefit: more
uniform/less API churn.
Implementation (I have a prototype):
 - LSP $/cancelRequest would move to JSONRPCDispatcher so it can cut across
all methods. Cleanup of TaskHandles would be handled with a context
destructor.
 - other users of ClangdServer would set up cancellation in the same way:
by creating a task handle and calling setCurrentTask() before invoking a
request. Or they can not do so if they don't support cancellation
(isCancelled() returns false if there's no task).
This is very similar to golang context cancellation.

3) TUScheduler should be cancellation-aware
This seems like an easy, cross-cutting win, but we should measure.

4) We should hide the Task object - it adds API noise and it offers too
many facilities to the wrong actors.
(maybe this is controversial, and somewhat less related).
 - Task::isCancelled() appears to be redundant, you never want to check
another task's status and checking for cancellation in a tight loop is an
antipattern that doesn't seem worth optimizing for
 - cancel() is only for the creator of a task (current API enforces this)
 - TaskHandle/ConstTaskHandle/getCurrentTask just exist to support exposing
these details
 - most actions have obvious extensions to cases where there is no active
task, but the current API makes this awkward
So this would leave us with something like (modulo names):
bool isTaskCancelled();
using Canceler = std::function<void()>;
pair<Context, Canceler> startCancelableTask();
(again, this borrows heavily from go

5) Random thought: we could support deadlines (automatic cancellation after
some time), this is useful in hosted scenarios though probably not for
standalone workstation use.

WDYT?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/clangd-dev/attachments/20180912/1fa92f1b/attachment.html>