[PATCH] D51475: [clangd] Load YAML static index asynchronously.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clangd/tool/ClangdMain.cpp:86
+      return nullptr;
+    Index = AsyncLoad.get();
+    return Index.get();
----------------
ilya-biryukov wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > ioeric wrote:
> > > > ilya-biryukov wrote:
> > > > > I believe is a data race (multiple threads may run this line concurrently).
> > > > > You would want some synchronization around this, `std::shared_future` could be a good fit
> > > > Nice catch. 
> > > > 
> > > > I am a bit hesitated about using `shared_future` though. It could potentially get rid of the mutex but would require much more careful use. For example, `Index` can be set to the same value repeatedly, which makes me a bit nervous.
> > > The following pattern should give proper results:
> > > ```
> > > class AsyncIndex : public Index {
> > > public:
> > >   AsyncIndex(std::shared_future<Index> IndexFut);
> > >   //....
> > > private;
> > >   const SymbolIndex *index() const {
> > >      if (IndexFut.wait(0) != ready)
> > >        return nullptr;
> > >      return &IndexFut.get();
> > >   }
> > > 
> > >   std::shared_future<Index>IndexFut;
> > > };
> > > 
> > > 
> > > AsyncIndex AI(std::async([]() { /* load the index here */ return Index; });
> > > ```
> > My concern with this approach is that we are still calling `wait` even though it's not necessary once Index has been loaded.
> Maybe not bother before we know it causes actual performance issues? My bet is that it would never become a bottleneck, given the amount of work we're doing in addition to every call here.
Sure, the performance overhead is small in both cases. But I would still try to avoid `wait` e.g. OS can decide to `yield` even if the timeout is 0. 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51475