[PATCH] D90750: [clangd] Introduce ProjectAwareIndex

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This looks really sensible to me! Nice you managed to reuse Memoize, there's very little ugly caching/locking left.



================
Comment at: clang-tools-extra/clangd/index/ProjectAware.cpp:81
+    return IndexForProject.get(
+        External.Location, [Server = External.Location,
+                            MP = llvm::StringRef(External.MountPoint), this] {
----------------
I think the cache key needs a bit more consideration.

It's unlikely that a filename will collide with a host/port, but they are currently sitting in the same namespace.

More worryingly, the mountpoint is used in the lambda body but not in the cache key - are we sure this is safe?

The clearest way to enforce this is to make index creation use the cache key only, and not other data from the config. Then you have to include what you're going to use.


================
Comment at: clang-tools-extra/clangd/index/ProjectAware.cpp:81
+    return IndexForProject.get(
+        External.Location, [Server = External.Location,
+                            MP = llvm::StringRef(External.MountPoint), this] {
----------------
sammccall wrote:
> I think the cache key needs a bit more consideration.
> 
> It's unlikely that a filename will collide with a host/port, but they are currently sitting in the same namespace.
> 
> More worryingly, the mountpoint is used in the lambda body but not in the cache key - are we sure this is safe?
> 
> The clearest way to enforce this is to make index creation use the cache key only, and not other data from the config. Then you have to include what you're going to use.
as discussed offline, allowing the config -> cache key and cache key -> index operations to be injectable is the key to testing I think


================
Comment at: clang-tools-extra/clangd/index/ProjectAware.cpp:100
+          });
+          addIndex(std::move(NewIndex));
+          return PlaceHolder;
----------------
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.


================
Comment at: clang-tools-extra/clangd/index/ProjectAware.cpp:119
+  SymbolIndex *Result = nullptr;
+  auto PushIndex = [this, &Result](SymbolIndex *Idx) {
+    if (!Result) {
----------------
consider moving this to `unique_ptr<MergedIndex> MergedIndex::create(ArrayRef<unique_ptr<SymbolIndex>>)` or so?

Not sure if this is something we'd want to use from ClangdMain...


================
Comment at: clang-tools-extra/clangd/index/ProjectAware.cpp:128
+  std::lock_guard<std::mutex> Lock(Mu);
+  for (const auto &Idx : IndexStorage) {
+    if (Idx.get() == Primary)
----------------
with the helper described above, the Primary ordering could be maybe better expressed as std::stable_partition


================
Comment at: clang-tools-extra/clangd/index/ProjectAware.h:30
+
+class ProjectAwareIndex : public SymbolIndex {
+public:
----------------
Is there a reason this class needs to be public?
I think we can expose factories returning SymbolIndex only


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90750