[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL341318: [clangd] Factor out the data-swapping functionality 
from MemIndex/DexIndex. (authored by sammccall, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D51422?vs=163715=163716#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D51422

Files:
  clang-tools-extra/trunk/clangd/index/FileIndex.cpp
  clang-tools-extra/trunk/clangd/index/FileIndex.h
  clang-tools-extra/trunk/clangd/index/Index.cpp
  clang-tools-extra/trunk/clangd/index/Index.h
  clang-tools-extra/trunk/clangd/index/MemIndex.cpp
  clang-tools-extra/trunk/clangd/index/MemIndex.h
  clang-tools-extra/trunk/clangd/index/dex/DexIndex.cpp
  clang-tools-extra/trunk/clangd/index/dex/DexIndex.h
  clang-tools-extra/trunk/unittests/clangd/DexIndexTests.cpp
  clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
  clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp
  clang-tools-extra/trunk/unittests/clangd/TestIndex.cpp
  clang-tools-extra/trunk/unittests/clangd/TestIndex.h

Index: clang-tools-extra/trunk/clangd/index/dex/DexIndex.h
===
--- clang-tools-extra/trunk/clangd/index/dex/DexIndex.h
+++ clang-tools-extra/trunk/clangd/index/dex/DexIndex.h
@@ -25,7 +25,6 @@
 #include "Iterator.h"
 #include "Token.h"
 #include "Trigram.h"
-#include 
 
 namespace clang {
 namespace clangd {
@@ -39,12 +38,24 @@
 // index on disk and then load it if available.
 class DexIndex : public SymbolIndex {
 public:
-  /// \brief (Re-)Build index for `Symbols`. All symbol pointers must remain
-  /// accessible as long as `Symbols` is kept alive.
-  void build(std::shared_ptr> Syms);
-
-  /// \brief Build index from a symbol slab.
-  static std::unique_ptr build(SymbolSlab Slab);
+  // All symbols must outlive this index.
+  template  DexIndex(Range &) {
+for (auto & : Symbols)
+  this->Symbols.push_back();
+buildIndex();
+  }
+  // Symbols are owned by BackingData, Index takes ownership.
+  template 
+  DexIndex(Range &, Payload &)
+  : DexIndex(std::forward(Symbols)) {
+KeepAlive = std::shared_ptr(
+std::make_shared(std::move(BackingData)), nullptr);
+  }
+
+  /// Builds an index from a slab. The index takes ownership of the slab.
+  static std::unique_ptr build(SymbolSlab Slab) {
+return llvm::make_unique(Slab, std::move(Slab));
+  }
 
   bool
   fuzzyFind(const FuzzyFindRequest ,
@@ -60,19 +71,19 @@
   size_t estimateMemoryUsage() const override;
 
 private:
+  void buildIndex();
 
-  mutable std::mutex Mutex;
-
-  std::shared_ptr> Symbols /*GUARDED_BY(Mutex)*/;
-  llvm::DenseMap LookupTable /*GUARDED_BY(Mutex)*/;
-  llvm::DenseMap SymbolQuality /*GUARDED_BY(Mutex)*/;
+  std::vector Symbols;
+  llvm::DenseMap LookupTable;
+  llvm::DenseMap SymbolQuality;
   // Inverted index is a mapping from the search token to the posting list,
   // which contains all items which can be characterized by such search token.
   // For example, if the search token is scope "std::", the corresponding
   // posting list would contain all indices of symbols defined in namespace std.
   // Inverted index is used to retrieve posting lists which are processed during
   // the fuzzyFind process.
-  llvm::DenseMap InvertedIndex /*GUARDED_BY(Mutex)*/;
+  llvm::DenseMap InvertedIndex;
+  std::shared_ptr KeepAlive; // poor man's move-only std::any
 };
 
 } // namespace dex
Index: clang-tools-extra/trunk/clangd/index/dex/DexIndex.cpp
===
--- clang-tools-extra/trunk/clangd/index/dex/DexIndex.cpp
+++ clang-tools-extra/trunk/clangd/index/dex/DexIndex.cpp
@@ -36,48 +36,30 @@
 
 } // namespace
 
-void DexIndex::build(std::shared_ptr> Syms) {
-  llvm::DenseMap TempLookupTable;
-  llvm::DenseMap TempSymbolQuality;
-  for (const Symbol *Sym : *Syms) {
-TempLookupTable[Sym->ID] = Sym;
-TempSymbolQuality[Sym] = quality(*Sym);
+void DexIndex::buildIndex() {
+  for (const Symbol *Sym : Symbols) {
+LookupTable[Sym->ID] = Sym;
+SymbolQuality[Sym] = quality(*Sym);
   }
 
   // Symbols are sorted by symbol qualities so that items in the posting lists
   // are stored in the descending order of symbol quality.
-  std::sort(begin(*Syms), end(*Syms),
+  std::sort(begin(Symbols), end(Symbols),
 [&](const Symbol *LHS, const Symbol *RHS) {
-  return TempSymbolQuality[LHS] > TempSymbolQuality[RHS];
+  return SymbolQuality[LHS] > SymbolQuality[RHS];
 });
-  llvm::DenseMap TempInvertedIndex;
+
   // Populate TempInvertedIndex with posting lists for index symbols.
-  for (DocID SymbolRank = 0; SymbolRank < Syms->size(); ++SymbolRank) {
-const auto *Sym = (*Syms)[SymbolRank];
+  for (DocID SymbolRank = 0; SymbolRank < Symbols.size(); ++SymbolRank) {
+const auto *Sym = Symbols[SymbolRank];
 for (const auto  : 

[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.

[Eric Liu via Phabricator via cfe-commits]

ioeric accepted this revision.
ioeric added inline comments.



Comment at: clangd/index/FileIndex.h:47
+  // The shared_ptr keeps the symbols alive.
+  std::shared_ptr buildMemIndex();
 

kbobyrev wrote:
> sammccall wrote:
> > ioeric wrote:
> > > Maybe avoid hardcoding the index name, so that we could potentially 
> > > switch to use a different index implementation?
> > > 
> > > We might also want to allow user to specify different index 
> > > implementations for file index e.g. main file dynamic index might prefer 
> > > MemIndex while Dex might be a better choice for the preamble index. 
> > I don't think "returns an index of unspecified implementation" is the best 
> > contract. MemIndex and DexIndex will both continue to exist, and they have 
> > different performance characteristics (roughly fast build vs fast query). 
> > So it should be up to the caller, no?
> We could make the index type a template parameter to allow users decide which 
> one they want (also, this could be have a default value, e.g. `MemIndex` and 
> later `DexIndex`). Would that be a viable solution?
Provide one interface for each index implementation sounds good to me. Thanks 
for the clarification!



Comment at: clangd/index/Index.h:15
 #include "clang/Lex/Lexer.h"
+#include "llvm/ADT/Any.h"
 #include "llvm/ADT/DenseMap.h"

nit: no longer needed?



Comment at: clangd/index/dex/DexIndex.h:42
+  // All symbols must outlive this index.
+  template  DexIndex(Range &) {
+for (auto & : Symbols)

sammccall wrote:
> ioeric wrote:
> > sammccall wrote:
> > > ioeric wrote:
> > > > Why is this constructor needed? I think this could restrict the 
> > > > flexibility of DexIndex initialization, in case we need to pass in 
> > > > extra parameters here.
> > > I'm not sure exactly what you mean here.
> > > 
> > > We need a way to specify the symbols to be indexed.
> > > Because these are now immutable, doing that in the constructor if 
> > > possible is best.
> > > 
> > > Previously this was a vector, but that sometimes required 
> > > us to construct that big vector, dereference all those pointers, and 
> > > throw away the vector. This signature is strictly more general (if you 
> > > have a vector of pointers, you can pass `make_pointee_range > > Symbol>`)
> > > 
> > > > in case we need to pass in extra parameters here.
> > > What stops us adding more parameters?
> > > What stops us adding more parameters?
> > I thought this template was added so that we could use it as a drop-in 
> > replacement of `MemIndex` (with the same signature) e.g. in `FileIndex`? I 
> > might have overthought though.
> > 
> > Thanks for the explanation!
> Sure, Dex and MemIndex had the same constructor signatures as each other 
> before this patch, and they have the same as each other after this patch.
> 
> That makes it convenient to use them interchangeably, but there's no hard 
> requirement (no construction from templates etc).
Sounds good.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51422



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

OK, I think this is good to go again.
(A couple of seemingly-unrelated fixes to SymbolOccurrenceKind that I ran into 
in tests)




Comment at: clangd/index/FileIndex.h:47
+  // The shared_ptr keeps the symbols alive.
+  std::shared_ptr buildMemIndex();
 

ioeric wrote:
> kbobyrev wrote:
> > sammccall wrote:
> > > ioeric wrote:
> > > > Maybe avoid hardcoding the index name, so that we could potentially 
> > > > switch to use a different index implementation?
> > > > 
> > > > We might also want to allow user to specify different index 
> > > > implementations for file index e.g. main file dynamic index might 
> > > > prefer MemIndex while Dex might be a better choice for the preamble 
> > > > index. 
> > > I don't think "returns an index of unspecified implementation" is the 
> > > best contract. MemIndex and DexIndex will both continue to exist, and 
> > > they have different performance characteristics (roughly fast build vs 
> > > fast query). So it should be up to the caller, no?
> > We could make the index type a template parameter to allow users decide 
> > which one they want (also, this could be have a default value, e.g. 
> > `MemIndex` and later `DexIndex`). Would that be a viable solution?
> Provide one interface for each index implementation sounds good to me. Thanks 
> for the clarification!
I'm not really sure what problem templates solve there?
If a caller wants a Dex index, we can just add a `buildDexIndex()` function.
Templating and requiring these to always have the same constructor parameters 
seems fragile and unneccesary unless the dependency from FileIndex->Dex is 
really bad for some reason.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51422



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 163715.
sammccall added a comment.

Remove some more shared_ptr occurrences that aren't needed anymore
Update comments


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51422

Files:
  clangd/index/FileIndex.cpp
  clangd/index/FileIndex.h
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/index/MemIndex.cpp
  clangd/index/MemIndex.h
  clangd/index/dex/DexIndex.cpp
  clangd/index/dex/DexIndex.h
  unittests/clangd/DexIndexTests.cpp
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/IndexTests.cpp
  unittests/clangd/TestIndex.cpp
  unittests/clangd/TestIndex.h

Index: unittests/clangd/TestIndex.h
===
--- unittests/clangd/TestIndex.h
+++ unittests/clangd/TestIndex.h
@@ -23,26 +23,11 @@
 // Creates Symbol instance and sets SymbolID to given QualifiedName.
 Symbol symbol(llvm::StringRef QName);
 
-// Bundles symbol pointers with the actual symbol slab the pointers refer to in
-// order to ensure that the slab isn't destroyed while it's used by and index.
-struct SlabAndPointers {
-  SymbolSlab Slab;
-  std::vector Pointers;
-};
+// Create a slab of symbols with the given qualified names as IDs and names.
+SymbolSlab generateSymbols(std::vector QualifiedNames);
 
-// Create a slab of symbols with the given qualified names as both IDs and
-// names. The life time of the slab is managed by the returned shared pointer.
-// If \p WeakSymbols is provided, it will be pointed to the managed object in
-// the returned shared pointer.
-std::shared_ptr>
-generateSymbols(std::vector QualifiedNames,
-std::weak_ptr *WeakSymbols = nullptr);
-
-// Create a slab of symbols with IDs and names [Begin, End], otherwise identical
-// to the `generateSymbols` above.
-std::shared_ptr>
-generateNumSymbols(int Begin, int End,
-   std::weak_ptr *WeakSymbols = nullptr);
+// Create a slab of symbols with IDs and names [Begin, End].
+SymbolSlab generateNumSymbols(int Begin, int End);
 
 // Returns fully-qualified name out of given symbol.
 std::string getQualifiedName(const Symbol );
Index: unittests/clangd/TestIndex.cpp
===
--- unittests/clangd/TestIndex.cpp
+++ unittests/clangd/TestIndex.cpp
@@ -26,30 +26,18 @@
   return Sym;
 }
 
-std::shared_ptr>
-generateSymbols(std::vector QualifiedNames,
-std::weak_ptr *WeakSymbols) {
+SymbolSlab generateSymbols(std::vector QualifiedNames) {
   SymbolSlab::Builder Slab;
   for (llvm::StringRef QName : QualifiedNames)
 Slab.insert(symbol(QName));
-
-  auto Storage = std::make_shared();
-  Storage->Slab = std::move(Slab).build();
-  for (const auto  : Storage->Slab)
-Storage->Pointers.push_back();
-  if (WeakSymbols)
-*WeakSymbols = Storage;
-  auto *Pointers = >Pointers;
-  return {std::move(Storage), Pointers};
+  return std::move(Slab).build();
 }
 
-std::shared_ptr>
-generateNumSymbols(int Begin, int End,
-   std::weak_ptr *WeakSymbols) {
+SymbolSlab generateNumSymbols(int Begin, int End) {
   std::vector Names;
   for (int i = Begin; i <= End; i++)
 Names.push_back(std::to_string(i));
-  return generateSymbols(Names, WeakSymbols);
+  return generateSymbols(Names);
 }
 
 std::string getQualifiedName(const Symbol ) {
Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -17,18 +17,16 @@
 #include "index/Merge.h"
 #include "gtest/gtest.h"
 
+using testing::AllOf;
+using testing::ElementsAre;
 using testing::Pointee;
 using testing::UnorderedElementsAre;
-using testing::AllOf;
+using namespace llvm;
 
 namespace clang {
 namespace clangd {
 namespace {
 
-std::shared_ptr emptyOccurrences() {
-  return llvm::make_unique();
-}
-
 MATCHER_P(Named, N, "") { return arg.Name == N; }
 MATCHER_P(OccurrenceRange, Range, "") {
   return std::tie(arg.Location.Start.Line, arg.Location.Start.Column,
@@ -54,155 +52,139 @@
 EXPECT_THAT(*S.find(SymbolID(Sym)), Named(Sym));
 }
 
-TEST(MemIndexTest, MemIndexSymbolsRecycled) {
-  MemIndex I;
-  std::weak_ptr Symbols;
-  I.build(generateNumSymbols(0, 10, ), emptyOccurrences());
-  FuzzyFindRequest Req;
-  Req.Query = "7";
-  EXPECT_THAT(match(I, Req), UnorderedElementsAre("7"));
+TEST(SwapIndexTest, OldIndexRecycled) {
+  auto Token = std::make_shared();
+  std::weak_ptr WeakToken = Token;
 
-  EXPECT_FALSE(Symbols.expired());
-  // Release old symbols.
-  I.build(generateNumSymbols(0, 0), emptyOccurrences());
-  EXPECT_TRUE(Symbols.expired());
+  SwapIndex S(make_unique(SymbolSlab(), MemIndex::OccurrenceMap(),
+std::move(Token)));
+  EXPECT_FALSE(WeakToken.expired()); // Current MemIndex keeps it alive.
+  S.reset(make_unique());  // Now the MemIndex is destroyed.
+  EXPECT_TRUE(WeakToken.expired());  // So the token is too.
 }
 
 

[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 163714.
sammccall added a comment.

Instead of returning shared_ptr, our implementations that have
backing data gain the ability to own that data (without coupling to its form).
Based on offline discussion with @ioeric.

Rebase to pick up SymbolOccurrence changes. The SymbolOccurrence parts of this
patch aren't terribly clean, but I think that's a problem for another day.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51422

Files:
  clangd/index/FileIndex.cpp
  clangd/index/FileIndex.h
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/index/MemIndex.cpp
  clangd/index/MemIndex.h
  clangd/index/dex/DexIndex.cpp
  clangd/index/dex/DexIndex.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/DexIndexTests.cpp
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/IndexTests.cpp
  unittests/clangd/TestIndex.cpp
  unittests/clangd/TestIndex.h

Index: unittests/clangd/TestIndex.h
===
--- unittests/clangd/TestIndex.h
+++ unittests/clangd/TestIndex.h
@@ -23,26 +23,11 @@
 // Creates Symbol instance and sets SymbolID to given QualifiedName.
 Symbol symbol(llvm::StringRef QName);
 
-// Bundles symbol pointers with the actual symbol slab the pointers refer to in
-// order to ensure that the slab isn't destroyed while it's used by and index.
-struct SlabAndPointers {
-  SymbolSlab Slab;
-  std::vector Pointers;
-};
+// Create a slab of symbols with the given qualified names as IDs and names.
+SymbolSlab generateSymbols(std::vector QualifiedNames);
 
-// Create a slab of symbols with the given qualified names as both IDs and
-// names. The life time of the slab is managed by the returned shared pointer.
-// If \p WeakSymbols is provided, it will be pointed to the managed object in
-// the returned shared pointer.
-std::shared_ptr>
-generateSymbols(std::vector QualifiedNames,
-std::weak_ptr *WeakSymbols = nullptr);
-
-// Create a slab of symbols with IDs and names [Begin, End], otherwise identical
-// to the `generateSymbols` above.
-std::shared_ptr>
-generateNumSymbols(int Begin, int End,
-   std::weak_ptr *WeakSymbols = nullptr);
+// Create a slab of symbols with IDs and names [Begin, End].
+SymbolSlab generateNumSymbols(int Begin, int End);
 
 // Returns fully-qualified name out of given symbol.
 std::string getQualifiedName(const Symbol );
Index: unittests/clangd/TestIndex.cpp
===
--- unittests/clangd/TestIndex.cpp
+++ unittests/clangd/TestIndex.cpp
@@ -26,30 +26,18 @@
   return Sym;
 }
 
-std::shared_ptr>
-generateSymbols(std::vector QualifiedNames,
-std::weak_ptr *WeakSymbols) {
+SymbolSlab generateSymbols(std::vector QualifiedNames) {
   SymbolSlab::Builder Slab;
   for (llvm::StringRef QName : QualifiedNames)
 Slab.insert(symbol(QName));
-
-  auto Storage = std::make_shared();
-  Storage->Slab = std::move(Slab).build();
-  for (const auto  : Storage->Slab)
-Storage->Pointers.push_back();
-  if (WeakSymbols)
-*WeakSymbols = Storage;
-  auto *Pointers = >Pointers;
-  return {std::move(Storage), Pointers};
+  return std::move(Slab).build();
 }
 
-std::shared_ptr>
-generateNumSymbols(int Begin, int End,
-   std::weak_ptr *WeakSymbols) {
+SymbolSlab generateNumSymbols(int Begin, int End) {
   std::vector Names;
   for (int i = Begin; i <= End; i++)
 Names.push_back(std::to_string(i));
-  return generateSymbols(Names, WeakSymbols);
+  return generateSymbols(Names);
 }
 
 std::string getQualifiedName(const Symbol ) {
Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -17,18 +17,16 @@
 #include "index/Merge.h"
 #include "gtest/gtest.h"
 
+using testing::AllOf;
+using testing::ElementsAre;
 using testing::Pointee;
 using testing::UnorderedElementsAre;
-using testing::AllOf;
+using namespace llvm;
 
 namespace clang {
 namespace clangd {
 namespace {
 
-std::shared_ptr emptyOccurrences() {
-  return llvm::make_unique();
-}
-
 MATCHER_P(Named, N, "") { return arg.Name == N; }
 MATCHER_P(OccurrenceRange, Range, "") {
   return std::tie(arg.Location.Start.Line, arg.Location.Start.Column,
@@ -54,155 +52,139 @@
 EXPECT_THAT(*S.find(SymbolID(Sym)), Named(Sym));
 }
 
-TEST(MemIndexTest, MemIndexSymbolsRecycled) {
-  MemIndex I;
-  std::weak_ptr Symbols;
-  I.build(generateNumSymbols(0, 10, ), emptyOccurrences());
-  FuzzyFindRequest Req;
-  Req.Query = "7";
-  EXPECT_THAT(match(I, Req), UnorderedElementsAre("7"));
+TEST(SwapIndexTest, OldIndexRecycled) {
+  auto Token = std::make_shared();
+  std::weak_ptr WeakToken = Token;
 
-  EXPECT_FALSE(Symbols.expired());
-  // Release old symbols.
-  I.build(generateNumSymbols(0, 0), emptyOccurrences());
-  EXPECT_TRUE(Symbols.expired());
+  

[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.

[Kirill Bobyrev via Phabricator via cfe-commits]

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

Other than a hard-coded `buildMemIndex()` in `FileIndex`, I don't have any 
concerns. That's just a tiny piece, if @ioeric doesn't have any concerns about 
that too, I think it's good to land.

The code should look cleaner in multiple places now, thanks for the patch!




Comment at: clangd/index/FileIndex.h:47
+  // The shared_ptr keeps the symbols alive.
+  std::shared_ptr buildMemIndex();
 

sammccall wrote:
> ioeric wrote:
> > Maybe avoid hardcoding the index name, so that we could potentially switch 
> > to use a different index implementation?
> > 
> > We might also want to allow user to specify different index implementations 
> > for file index e.g. main file dynamic index might prefer MemIndex while Dex 
> > might be a better choice for the preamble index. 
> I don't think "returns an index of unspecified implementation" is the best 
> contract. MemIndex and DexIndex will both continue to exist, and they have 
> different performance characteristics (roughly fast build vs fast query). So 
> it should be up to the caller, no?
We could make the index type a template parameter to allow users decide which 
one they want (also, this could be have a default value, e.g. `MemIndex` and 
later `DexIndex`). Would that be a viable solution?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51422



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clangd/index/FileIndex.h:47
+  // The shared_ptr keeps the symbols alive.
+  std::shared_ptr buildMemIndex();
 

ioeric wrote:
> Maybe avoid hardcoding the index name, so that we could potentially switch to 
> use a different index implementation?
> 
> We might also want to allow user to specify different index implementations 
> for file index e.g. main file dynamic index might prefer MemIndex while Dex 
> might be a better choice for the preamble index. 
I don't think "returns an index of unspecified implementation" is the best 
contract. MemIndex and DexIndex will both continue to exist, and they have 
different performance characteristics (roughly fast build vs fast query). So it 
should be up to the caller, no?



Comment at: clangd/index/MemIndex.h:30
+  /// Builds an index from a slab. The shared_ptr manages the slab's lifetime.
+  static std::shared_ptr build(SymbolSlab Slab);
 

ioeric wrote:
> sammccall wrote:
> > ioeric wrote:
> > > (It's a bit unfortunate that this has to return `shared_ptr` now)
> > Since some of the resources it owns has a shared lifetime, this is really 
> > just reflecting reality I think. Whether that's visible or invisible seems 
> > like a wash to me.
> This is only true when this is used with `SwapIndex` right? For example, a 
> static Dex/Mem index would probbaly have `unique_ptr` ownership.
Dex and MemIndex don't own the symbols.

If S is a SymbolSlab:
 - `MemIndex(S)` (or `make_unique(S)`) is a MemIndex that doesn't 
know about the storage and doesn't own the symbols
 - `MemIndex::build(move(S))` is a `shared_ptr` that owns the 
storage. It's the `shared_ptr`, not the `MemIndex`, that manages the storage.

So for a static index, you could either hand off the ownership of the slab by 
calling build(), or keep it yourself and call the constructor. The former seems 
more convenient in ClangdMain.

We could write this differently, e.g. as `std::shared_ptr = 
tieStorageToIndex(std::move(S), make_unique(S));`. The API here 
(`MemIndex::build()`) is intended to make common use cases easy, and keep it 
fairly similar to before for migration purposes. Is it too confusing?



Comment at: clangd/index/dex/DexIndex.h:42
+  // All symbols must outlive this index.
+  template  DexIndex(Range &) {
+for (auto & : Symbols)

ioeric wrote:
> sammccall wrote:
> > ioeric wrote:
> > > Why is this constructor needed? I think this could restrict the 
> > > flexibility of DexIndex initialization, in case we need to pass in extra 
> > > parameters here.
> > I'm not sure exactly what you mean here.
> > 
> > We need a way to specify the symbols to be indexed.
> > Because these are now immutable, doing that in the constructor if possible 
> > is best.
> > 
> > Previously this was a vector, but that sometimes required us 
> > to construct that big vector, dereference all those pointers, and throw 
> > away the vector. This signature is strictly more general (if you have a 
> > vector of pointers, you can pass `make_pointee_range`)
> > 
> > > in case we need to pass in extra parameters here.
> > What stops us adding more parameters?
> > What stops us adding more parameters?
> I thought this template was added so that we could use it as a drop-in 
> replacement of `MemIndex` (with the same signature) e.g. in `FileIndex`? I 
> might have overthought though.
> 
> Thanks for the explanation!
Sure, Dex and MemIndex had the same constructor signatures as each other before 
this patch, and they have the same as each other after this patch.

That makes it convenient to use them interchangeably, but there's no hard 
requirement (no construction from templates etc).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51422



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/index/MemIndex.h:30
+  /// Builds an index from a slab. The shared_ptr manages the slab's lifetime.
+  static std::shared_ptr build(SymbolSlab Slab);
 

sammccall wrote:
> ioeric wrote:
> > (It's a bit unfortunate that this has to return `shared_ptr` now)
> Since some of the resources it owns has a shared lifetime, this is really 
> just reflecting reality I think. Whether that's visible or invisible seems 
> like a wash to me.
This is only true when this is used with `SwapIndex` right? For example, a 
static Dex/Mem index would probbaly have `unique_ptr` ownership.



Comment at: clangd/index/dex/DexIndex.h:42
+  // All symbols must outlive this index.
+  template  DexIndex(Range &) {
+for (auto & : Symbols)

sammccall wrote:
> ioeric wrote:
> > Why is this constructor needed? I think this could restrict the flexibility 
> > of DexIndex initialization, in case we need to pass in extra parameters 
> > here.
> I'm not sure exactly what you mean here.
> 
> We need a way to specify the symbols to be indexed.
> Because these are now immutable, doing that in the constructor if possible is 
> best.
> 
> Previously this was a vector, but that sometimes required us 
> to construct that big vector, dereference all those pointers, and throw away 
> the vector. This signature is strictly more general (if you have a vector of 
> pointers, you can pass `make_pointee_range`)
> 
> > in case we need to pass in extra parameters here.
> What stops us adding more parameters?
> What stops us adding more parameters?
I thought this template was added so that we could use it as a drop-in 
replacement of `MemIndex` (with the same signature) e.g. in `FileIndex`? I 
might have overthought though.

Thanks for the explanation!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51422



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clangd/index/Index.h:416
+  // until the call returns (even if reset() is called).
+  bool fuzzyFind(const FuzzyFindRequest &,
+ llvm::function_ref) const override;

kbobyrev wrote:
> Do we want these functions to be `final`? Since `SwapIndex` is a wrapper 
> around an immutable index structure, I believe it would be unlikely that 
> anything would derive from it.
Maybe unlikely but I don't see a strong reason to enforce one way or the other. 
(We rarely use final).

One example of where we'll do it: FileIndex inherits SwapIndex, but it should 
override `estimateMemoryUsage` once that's fixed to incorporate backing symbols.



Comment at: clangd/index/MemIndex.h:30
+  /// Builds an index from a slab. The shared_ptr manages the slab's lifetime.
+  static std::shared_ptr build(SymbolSlab Slab);
 

ioeric wrote:
> (It's a bit unfortunate that this has to return `shared_ptr` now)
Since some of the resources it owns has a shared lifetime, this is really just 
reflecting reality I think. Whether that's visible or invisible seems like a 
wash to me.



Comment at: clangd/index/dex/DexIndex.h:42
+  // All symbols must outlive this index.
+  template  DexIndex(Range &) {
+for (auto & : Symbols)

ioeric wrote:
> Why is this constructor needed? I think this could restrict the flexibility 
> of DexIndex initialization, in case we need to pass in extra parameters here.
I'm not sure exactly what you mean here.

We need a way to specify the symbols to be indexed.
Because these are now immutable, doing that in the constructor if possible is 
best.

Previously this was a vector, but that sometimes required us to 
construct that big vector, dereference all those pointers, and throw away the 
vector. This signature is strictly more general (if you have a vector of 
pointers, you can pass `make_pointee_range`)

> in case we need to pass in extra parameters here.
What stops us adding more parameters?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51422



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/index/dex/DexIndex.h:42
+  // All symbols must outlive this index.
+  template  DexIndex(Range &) {
+for (auto & : Symbols)

Why is this constructor needed? I think this could restrict the flexibility of 
DexIndex initialization, in case we need to pass in extra parameters here.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51422



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.



Comment at: clangd/index/Index.h:416
+  // until the call returns (even if reset() is called).
+  bool fuzzyFind(const FuzzyFindRequest &,
+ llvm::function_ref) const override;

Do we want these functions to be `final`? Since `SwapIndex` is a wrapper around 
an immutable index structure, I believe it would be unlikely that anything 
would derive from it.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51422



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/index/FileIndex.h:47
+  // The shared_ptr keeps the symbols alive.
+  std::shared_ptr buildMemIndex();
 

Maybe avoid hardcoding the index name, so that we could potentially switch to 
use a different index implementation?

We might also want to allow user to specify different index implementations for 
file index e.g. main file dynamic index might prefer MemIndex while Dex might 
be a better choice for the preamble index. 



Comment at: clangd/index/MemIndex.h:30
+  /// Builds an index from a slab. The shared_ptr manages the slab's lifetime.
+  static std::shared_ptr build(SymbolSlab Slab);
 

(It's a bit unfortunate that this has to return `shared_ptr` now)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51422



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: kbobyrev.
Herald added subscribers: cfe-commits, kadircet, arphaman, mgrang, jkorous, 
MaskRay, ioeric, ilya-biryukov.

This is now handled by a wrapper class SwapIndex, so MemIndex/DexIndex can be
immutable and focus on their job.

Old and busted:
 I have a MemIndex, which holds a shared_ptr>, which keeps the
 symbol slab alive. I update by calling build(shared_ptr>).

New hotness: I have a SwapIndex, which holds a shared_ptr, which
 holds a MemIndex and also keeps any data backing it alive.
 I update by building a new MemIndex and calling SwapIndex::reset().

This resulted in a bunch of interface churn (some places previously using
unique_ptr should now be shared_ptr, and some using MemIndex() + MemIndex::build
should now call the static MemIndex::build factory instead).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51422

Files:
  clangd/index/FileIndex.cpp
  clangd/index/FileIndex.h
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/index/MemIndex.cpp
  clangd/index/MemIndex.h
  clangd/index/dex/DexIndex.cpp
  clangd/index/dex/DexIndex.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/DexIndexTests.cpp
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/IndexTests.cpp
  unittests/clangd/TestIndex.cpp
  unittests/clangd/TestIndex.h
  unittests/clangd/TestTU.cpp
  unittests/clangd/TestTU.h

Index: unittests/clangd/TestTU.h
===
--- unittests/clangd/TestTU.h
+++ unittests/clangd/TestTU.h
@@ -51,7 +51,7 @@
 
   ParsedAST build() const;
   SymbolSlab headerSymbols() const;
-  std::unique_ptr index() const;
+  std::shared_ptr index() const;
 };
 
 // Look up an index symbol by qualified name, which must be unique.
Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -48,7 +48,7 @@
   return indexAST(AST.getASTContext(), AST.getPreprocessorPtr());
 }
 
-std::unique_ptr TestTU::index() const {
+std::shared_ptr TestTU::index() const {
   return MemIndex::build(headerSymbols());
 }
 
Index: unittests/clangd/TestIndex.h
===
--- unittests/clangd/TestIndex.h
+++ unittests/clangd/TestIndex.h
@@ -23,26 +23,11 @@
 // Creates Symbol instance and sets SymbolID to given QualifiedName.
 Symbol symbol(llvm::StringRef QName);
 
-// Bundles symbol pointers with the actual symbol slab the pointers refer to in
-// order to ensure that the slab isn't destroyed while it's used by and index.
-struct SlabAndPointers {
-  SymbolSlab Slab;
-  std::vector Pointers;
-};
+// Create a slab of symbols with the given qualified names as IDs and names.
+SymbolSlab generateSymbols(std::vector QualifiedNames);
 
-// Create a slab of symbols with the given qualified names as both IDs and
-// names. The life time of the slab is managed by the returned shared pointer.
-// If \p WeakSymbols is provided, it will be pointed to the managed object in
-// the returned shared pointer.
-std::shared_ptr>
-generateSymbols(std::vector QualifiedNames,
-std::weak_ptr *WeakSymbols = nullptr);
-
-// Create a slab of symbols with IDs and names [Begin, End], otherwise identical
-// to the `generateSymbols` above.
-std::shared_ptr>
-generateNumSymbols(int Begin, int End,
-   std::weak_ptr *WeakSymbols = nullptr);
+// Create a slab of symbols with IDs and names [Begin, End].
+SymbolSlab generateNumSymbols(int Begin, int End);
 
 // Returns fully-qualified name out of given symbol.
 std::string getQualifiedName(const Symbol );
Index: unittests/clangd/TestIndex.cpp
===
--- unittests/clangd/TestIndex.cpp
+++ unittests/clangd/TestIndex.cpp
@@ -26,30 +26,18 @@
   return Sym;
 }
 
-std::shared_ptr>
-generateSymbols(std::vector QualifiedNames,
-std::weak_ptr *WeakSymbols) {
+SymbolSlab generateSymbols(std::vector QualifiedNames) {
   SymbolSlab::Builder Slab;
   for (llvm::StringRef QName : QualifiedNames)
 Slab.insert(symbol(QName));
-
-  auto Storage = std::make_shared();
-  Storage->Slab = std::move(Slab).build();
-  for (const auto  : Storage->Slab)
-Storage->Pointers.push_back();
-  if (WeakSymbols)
-*WeakSymbols = Storage;
-  auto *Pointers = >Pointers;
-  return {std::move(Storage), Pointers};
+  return std::move(Slab).build();
 }
 
-std::shared_ptr>
-generateNumSymbols(int Begin, int End,
-   std::weak_ptr *WeakSymbols) {
+SymbolSlab generateNumSymbols(int Begin, int End) {
   std::vector Names;
   for (int i = Begin; i <= End; i++)
 Names.push_back(std::to_string(i));
-  return generateSymbols(Names, WeakSymbols);
+  return generateSymbols(Names);
 }
 
 std::string getQualifiedName(const Symbol ) {
Index: unittests/clangd/IndexTests.cpp