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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D51475#1219197, @ilya-biryukov wrote:

> In https://reviews.llvm.org/D51475#1219184, @ioeric wrote:
>
> > The index loading can be slow. When using LLVM YAML index, I need to wait for >10s before clangd starts giving me anything useful. We could potentially speed up loading (e.g. replacing yaml), but the index can be arbitrary large. I think it's an improvement in general to be able to get clangd running before index is loaded.
>
>
> I would trade-off those 10 seconds for giving consistent experience (i.e. avoiding confusing the users with different modes of completion (with and without index, etc.)).
>  But not terribly opposed to that.


I think the inconsistency is benign and should be worth 10 seconds (which for me personally is loong...). Besides, this is just LLVM index; the loading could be arbitrary long depending on the corpus size.



================
Comment at: clangd/tool/ClangdMain.cpp:86
+      return nullptr;
+    Index = AsyncLoad.get();
+    return Index.get();
----------------
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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51475