[PATCH] D51996: [clangd] Simplify cancellation public API

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In https://reviews.llvm.org/D51996#1232459, @kadircet wrote:

> Seems a lot cleaner now, thanks!
>
> Do you plan to have other changes like moving control to JSONRPCDispatcher and recording timings for analysis on this patch?


https://reviews.llvm.org/D52004 is the JSONRPCDispatcher change.
Added a FIXME to the file comment about timings, hope to get to it soon.

> If not maybe we can add some fixme's so that we won't forget. Also the somewhat "caching" of cancellation token from previous implementation might still be useful in future if we really face "crowded" contexts and frequent cancellation checks, so maybe keep some notes about it?

I think the best plan for frequent cancellation checks is "don't do that" ;-) The situation is that you're checking for cancellation to save CPU, but the thing you're spending the CPU on is checking for cancellation...  I don't think the fix is to make cancellation checking cheaper! (Obviously cheaper is always better, but it would make the API worse here)
So i left a note on isCancelled(). We can revisit if we find cases where context traversal is really hurting us.



================
Comment at: clangd/Cancellation.h:29
+// 2. Library code that executes long-running work, and can exit early if the
+//   result is not needed.
 //
----------------
kadircet wrote:
> Maybe also mention propagating context into long-running work. runAsync does that implicitly and not sure if there will be other use cases that doesn't include it, but if there might be it would be nice to point it out as well.
Yeah, added this to the caveats.
This shouldn't be a problem in practice (at least for pure open-source clangd): LLVM doesn't have a lot of multithreading utilities so we mostly define our own, and those are context aware.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51996