[PATCH] D90750: [clangd] Introduce ProjectAwareIndex

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/index/ProjectAware.cpp:100
+          });
+          addIndex(std::move(NewIndex));
+          return PlaceHolder;
----------------
sammccall wrote:
> from memoize:
> 
> > Concurrent calls for the same key may run the
> > computation multiple times, but each call will return the same result.
> 
> so this side-effect may run multiple times too, resulting in multiple index copies getting leaked. This seems like it may be bad in the case of file indexes.
> 
> Why can't the memoize map own the indexes?
> This would still result in loading the index multiple times and throwing things away, hmm. Not sure how best to resolve this.
> so this side-effect may run multiple times too, resulting in multiple index copies getting leaked. This seems like it may be bad in the case of file indexes.

The only way i can see this happening is by synchronizing calls to `getIndex`.

> Why can't the memoize map own the indexes?

Main problem is, it returns a copy to the stored elements, rather than a reference to them. Which might be necessary in general to guarantee thread-safety. Since we can't copy unique_ptrs, I had to store them elsewhere and only keep pointers to it within the memoize map.


So both things considered, maybe we should just give up on memoize and have our own thread-safe cache here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90750