<div dir="ltr">Guess I was right about which piece would be controversial.<div>(For those missing context, Cider is a non-public editor that embeds ClangdServer)<br><div><br><div class="gmail_quote"><div dir="ltr">On Wed, Sep 12, 2018 at 11:28 AM Ilya Biryukov <<a href="mailto:ibiryukov@google.com">ibiryukov@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>Hi Sam,</div><div><br></div><div><br></div><div>A few things that come to mind:</div>>  - 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).<div><br></div><div>My stance is that explicit APIs for cancellation are better approach.</div></div></blockquote><div>I'd like to clarify what you mean by "explicit", because the *cancellation* API is explicit in both cases, and the *coordination* between the cancellation and the cancel-check is (to a first approximation) magic in both cases.</div><div><br></div><div>I think the biggest difference is whether the cancelability of an operation is represented in its static type (e.g. function return type). Is this what you mean?</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>They clearly state the contract of the API, it's impossible to accidentally reuse the existing task handle (ClangdServer is creating a new task every time) multiple times for different tasks from the client code and they are (arguably) easier to use from Cider.</div><div>The "task in the context" approach, OTOH, means users will have to read through the comments to even discover that the cancellation is there.</div></div></blockquote><div>I guess I don't see that as a bad thing? Cancellation is a cross-cutting concern that you mostly want to ignore, except at layers where: a) you can meaningfully abort early to save some work, or b) you want to start a chunk of work and maybe cancel it later.</div><div>Examples of why this seems right: </div><div> - there's no reason that a cancelable span should be coupled 1:1 with an LSP method</div><div> - whether a function will *respond* to cancellation is always a *dynamic* property. This is a QoI issue, not a contract.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>I may be missing the reasons on why the proposed approach is better. Any suggestions I'm missing?</div></div></blockquote><div>- It allows us to support cancellation of all methods without adding a bunch of bookkeeping to each one (or equivalent in templates)</div><div>- it allows embedders to support cancelling methods at the layer it makes sense without plumbing cancellation objects around</div><div>- it avoids divergences in APIs based on whether we've implemented early-exit for this operation or not</div><div>- putting values in the return type that are not the result of the computation surprises the reader</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>> 1) task should record the time of requested cancellation<br></div><div>+1 <br></div><div>> 3) TUScheduler should be cancellation-aware</div><div>+1 <br></div><div><br></div><div>> 4) We should hide the Task object.</div><div>> (again, this borrows heavily from go)</div><div>Go was definitely designed with cancellation (I assume of both RPCs and general computations?) in mind,</div></div></blockquote><div>For what it's worth, it wasn't - Context and cancellation was very much bolted on in the same way it was in clangd.</div><div>The only real difference is they decided not to use TLS for Context. But they decided not to signal cancelability explicitly in the API for the usual reasons (intermediate stack frames don't care).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>but my view on this is that in C++ it's more idiomatic to make the contracts like this explicit in the API. TaskHandle exists to make sure the callers of async APIs get an explicit object they can poke with, see the first comment about explicit vs implicit APIs.</div><div><br></div><div><div>> 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.</div><div>+1, unless it's trivial to do it on Cider side. In which case, maybe we could only support this in Cider to avoid adding the code to OS version that we won't use outside our environment.<br></div><br class="m_-2064177604064931322gmail-Apple-interchange-newline"></div><div><br></div><div><br class="m_-2064177604064931322gmail-Apple-interchange-newline"></div></div><br><div class="gmail_quote"><div dir="ltr">On Wed, Sep 12, 2018 at 10:57 AM Sam McCall <<a href="mailto:sammccall@google.com" target="_blank">sammccall@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>Hey Ilya and Kadir,</div><div><br></div><div>Was trying to understand how much we (can) win from cancellation, and what's involved in instrumenting a LSP method for cancellation.</div><div><br></div><div>Have a couple of proposals, wanted to discuss first them rather than sending a patch because they're separate but interact.</div><div><br></div><div>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).</div><div>Implementation is easy, just change the atomic<bool> to atomic<steady_clock::rep></div><div><br></div><div>2) we should support cancellation of any method, even if early bailout isn't yet implemented.</div><div>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.</div><div>Implementation (I have a prototype):</div><div> - LSP $/cancelRequest would move to JSONRPCDispatcher so it can cut across all methods. Cleanup of TaskHandles would be handled with a context destructor.</div><div> - 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).</div><div>This is very similar to golang context cancellation.</div><div><br></div><div>3) TUScheduler should be cancellation-aware</div><div>This seems like an easy, cross-cutting win, but we should measure.</div><div><br></div><div>4) We should hide the Task object - it adds API noise and it offers too many facilities to the wrong actors.</div><div>(maybe this is controversial, and somewhat less related).</div><div> - 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</div><div> - cancel() is only for the creator of a task (current API enforces this)</div><div> - TaskHandle/ConstTaskHandle/getCurrentTask just exist to support exposing these details<br></div><div> - most actions have obvious extensions to cases where there is no active task, but the current API makes this awkward</div><div>So this would leave us with something like (modulo names):</div><div>bool isTaskCancelled();</div><div>using Canceler = std::function<void()>;</div><div>pair<Context, Canceler> startCancelableTask();</div><div>(again, this borrows heavily from go</div><div><br></div><div>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.</div><div><br></div><div>WDYT?</div></div>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="m_-2064177604064931322gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div>Regards,</div><div>Ilya Biryukov</div></div></div></div></div>
</blockquote></div></div></div></div>