[PATCH] D94606: [clangd] Move DirBasedCDB broadcasting onto its own thread.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

thanks, lgtm!



================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:496
+  std::condition_variable CV;
+  std::atomic<bool> ShouldStop = {false}; // Must notify CV after writing.
+  std::deque<CDBLookupResult> Queue;
----------------
sammccall wrote:
> kadircet wrote:
> > nit: we already hold the lock within the loop while reading. it is nice that we no longer need to acquire the lock during destructor, but is it worth the extra mental load that comes with memory orderings? (also IIRC, default load/store behaviors are already acquire-release)
> Whoops, I split this patch in half and now this makes no sense.
> 
> The point is rather that we should interrupt the "evaluate the config for every file" loop (which will be part of process()) if ShouldStop is set. So we check this atomic inside that loop, without acquiring the lock.
> 
> (In practice maybe this is always "fast enough" that we should just run the whole loop, enqueue all the background indexing stuff, and then shut down afterwards?)
> 
> > (also IIRC, default load/store behaviors are already acquire-release)
> 
> Default is sequentially consistent, which is almost always more than we need.
> 
> ---
> 
> I've added a check to the existing loop (which probes directory caches), less necessary but shows the idea. If you prefer to drop this entirely that's OK too.
> The point is rather that we should interrupt the "evaluate the config for every file" loop (which will be part of process()) if ShouldStop is set. So we check this atomic inside that loop, without acquiring the lock.

ah okay now it makes sense.

> I've added a check to the existing loop (which probes directory caches), less necessary but shows the idea. If you prefer to drop this entirely that's OK too.

i suppose we could end up blocking the shutdown for quite some time in pathological cases due to disk io. so this looks like a good tradeoff.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94606/new/

https://reviews.llvm.org/D94606