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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
