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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In https://reviews.llvm.org/D51996#1232770, @ilya-biryukov wrote:

> > but the thing you're spending the CPU on is checking for cancellation...
>
> Unless checking for cancellation is really cheap (which is doable, I think).
>  We should probably hit some of those in practice before doing something, though.


My point is that if a few pointer traversals/comparisons is significant, then the loop is so fast that checking for cancellations doesn't make sense anyway - we're aiming to save 10s of ms, not a few hundred nanos.

Like you say, maybe there are important cases where we can't tell that we're calling frequently, or can't easily avoid that. We don't have any yet though, so optimizing this seems premature.



================
Comment at: clangd/Cancellation.h:81
+/// Always false if there is no active cancelable task.
+/// This isn't free (context lookup) - don't call it in a tight loop.
+bool isCancelled();
----------------
ilya-biryukov wrote:
> Allowing fast checking for cancellation seem important.
> E.g. if we want to cancel in the middle of parsing, we'll probably be faced with sema callbacks, frequency of which we don't control, so we'd better design something that's fast to check.
> 
> I see two ways to deal with this:
> - Keep the current design, but cache last access to context key.
> - Add an API to get something that can quickly check for cancellation, i.e. something that would hold a reference to resolved context key.
> 
> Both could be added later, though, and I don't have any data, it's just my intuition.
Yeah, we have a few options if we get there here.
Maybe the simplest is

```
if (LLVM_UNLIKELY(++Counter % 1000 == 0))
 if (isCancelled())
  return;
```
(obviously this is limited, but it depends what problem we're facing)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51996