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<vector<Symbol*>>, which keeps the symbol slab alive. I update by calling build(shared_ptr<vector<Symbol*>>). New hotness: I have a SwapIndex, which holds a shared_ptr<SymbolIndex>, 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<SymbolIndex> index() const; + std::shared_ptr<SymbolIndex> 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<SymbolIndex> TestTU::index() const { +std::shared_ptr<SymbolIndex> 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<const Symbol *> Pointers; -}; +// Create a slab of symbols with the given qualified names as IDs and names. +SymbolSlab generateSymbols(std::vector<std::string> 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<std::vector<const Symbol *>> -generateSymbols(std::vector<std::string> QualifiedNames, - std::weak_ptr<SlabAndPointers> *WeakSymbols = nullptr); - -// Create a slab of symbols with IDs and names [Begin, End], otherwise identical -// to the `generateSymbols` above. -std::shared_ptr<std::vector<const Symbol *>> -generateNumSymbols(int Begin, int End, - std::weak_ptr<SlabAndPointers> *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 &Sym); Index: unittests/clangd/TestIndex.cpp =================================================================== --- unittests/clangd/TestIndex.cpp +++ unittests/clangd/TestIndex.cpp @@ -26,30 +26,18 @@ return Sym; } -std::shared_ptr<std::vector<const Symbol *>> -generateSymbols(std::vector<std::string> QualifiedNames, - std::weak_ptr<SlabAndPointers> *WeakSymbols) { +SymbolSlab generateSymbols(std::vector<std::string> QualifiedNames) { SymbolSlab::Builder Slab; for (llvm::StringRef QName : QualifiedNames) Slab.insert(symbol(QName)); - - auto Storage = std::make_shared<SlabAndPointers>(); - Storage->Slab = std::move(Slab).build(); - for (const auto &Sym : Storage->Slab) - Storage->Pointers.push_back(&Sym); - if (WeakSymbols) - *WeakSymbols = Storage; - auto *Pointers = &Storage->Pointers; - return {std::move(Storage), Pointers}; + return std::move(Slab).build(); } -std::shared_ptr<std::vector<const Symbol *>> -generateNumSymbols(int Begin, int End, - std::weak_ptr<SlabAndPointers> *WeakSymbols) { +SymbolSlab generateNumSymbols(int Begin, int End) { std::vector<std::string> 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 &Sym) { Index: unittests/clangd/IndexTests.cpp =================================================================== --- unittests/clangd/IndexTests.cpp +++ unittests/clangd/IndexTests.cpp @@ -14,8 +14,10 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" +using testing::ElementsAre; using testing::Pointee; using testing::UnorderedElementsAre; +using namespace llvm; namespace clang { namespace clangd { @@ -39,152 +41,135 @@ EXPECT_THAT(*S.find(SymbolID(Sym)), Named(Sym)); } -TEST(MemIndexTest, MemIndexSymbolsRecycled) { - MemIndex I; - std::weak_ptr<SlabAndPointers> Symbols; - I.build(generateNumSymbols(0, 10, &Symbols)); - FuzzyFindRequest Req; - Req.Query = "7"; - EXPECT_THAT(match(I, Req), UnorderedElementsAre("7")); +TEST(SwapIndexTest, OldIndexRecycled) { + auto A = std::make_shared<MemIndex>(); + std::weak_ptr<SymbolIndex> WeakA = A; - EXPECT_FALSE(Symbols.expired()); - // Release old symbols. - I.build(generateNumSymbols(0, 0)); - EXPECT_TRUE(Symbols.expired()); + SwapIndex S(std::move(A)); + EXPECT_FALSE(WeakA.expired()); + S.reset(std::make_shared<MemIndex>()); + EXPECT_TRUE(WeakA.expired()); } TEST(MemIndexTest, MemIndexDeduplicate) { - auto Symbols = generateNumSymbols(0, 10); - - // Inject some duplicates and make sure we only match the same symbol once. - auto Sym = symbol("7"); - Symbols->push_back(&Sym); - Symbols->push_back(&Sym); - Symbols->push_back(&Sym); - + std::vector<Symbol> Symbols = {symbol("1"), symbol("2"), symbol("3"), + symbol("2") /* duplicate */}; FuzzyFindRequest Req; - Req.Query = "7"; - MemIndex I; - I.build(std::move(Symbols)); - auto Matches = match(I, Req); - EXPECT_EQ(Matches.size(), 1u); + Req.Query = "2"; + MemIndex I(Symbols); + EXPECT_THAT(match(I, Req), ElementsAre("2")); } TEST(MemIndexTest, MemIndexLimitedNumMatches) { - MemIndex I; - I.build(generateNumSymbols(0, 100)); + auto I = MemIndex::build(generateNumSymbols(0, 100)); FuzzyFindRequest Req; Req.Query = "5"; Req.MaxCandidateCount = 3; bool Incomplete; - auto Matches = match(I, Req, &Incomplete); + auto Matches = match(*I, Req, &Incomplete); EXPECT_EQ(Matches.size(), Req.MaxCandidateCount); EXPECT_TRUE(Incomplete); } TEST(MemIndexTest, FuzzyMatch) { - MemIndex I; - I.build( + auto I = MemIndex::build( generateSymbols({"LaughingOutLoud", "LionPopulation", "LittleOldLady"})); FuzzyFindRequest Req; Req.Query = "lol"; Req.MaxCandidateCount = 2; - EXPECT_THAT(match(I, Req), + EXPECT_THAT(match(*I, Req), UnorderedElementsAre("LaughingOutLoud", "LittleOldLady")); } TEST(MemIndexTest, MatchQualifiedNamesWithoutSpecificScope) { - MemIndex I; - I.build(generateSymbols({"a::y1", "b::y2", "y3"})); + auto I = MemIndex::build( + generateSymbols({"a::y1", "b::y2", "y3"})); FuzzyFindRequest Req; Req.Query = "y"; - EXPECT_THAT(match(I, Req), UnorderedElementsAre("a::y1", "b::y2", "y3")); + EXPECT_THAT(match(*I, Req), UnorderedElementsAre("a::y1", "b::y2", "y3")); } TEST(MemIndexTest, MatchQualifiedNamesWithGlobalScope) { - MemIndex I; - I.build(generateSymbols({"a::y1", "b::y2", "y3"})); + auto I = MemIndex::build( + generateSymbols({"a::y1", "b::y2", "y3"})); FuzzyFindRequest Req; Req.Query = "y"; Req.Scopes = {""}; - EXPECT_THAT(match(I, Req), UnorderedElementsAre("y3")); + EXPECT_THAT(match(*I, Req), UnorderedElementsAre("y3")); } TEST(MemIndexTest, MatchQualifiedNamesWithOneScope) { - MemIndex I; - I.build(generateSymbols({"a::y1", "a::y2", "a::x", "b::y2", "y3"})); + auto I = MemIndex::build( + generateSymbols({"a::y1", "a::y2", "a::x", "b::y2", "y3"})); FuzzyFindRequest Req; Req.Query = "y"; Req.Scopes = {"a::"}; - EXPECT_THAT(match(I, Req), UnorderedElementsAre("a::y1", "a::y2")); + EXPECT_THAT(match(*I, Req), UnorderedElementsAre("a::y1", "a::y2")); } TEST(MemIndexTest, MatchQualifiedNamesWithMultipleScopes) { - MemIndex I; - I.build(generateSymbols({"a::y1", "a::y2", "a::x", "b::y3", "y3"})); + auto I = MemIndex::build( + generateSymbols({"a::y1", "a::y2", "a::x", "b::y3", "y3"})); FuzzyFindRequest Req; Req.Query = "y"; Req.Scopes = {"a::", "b::"}; - EXPECT_THAT(match(I, Req), UnorderedElementsAre("a::y1", "a::y2", "b::y3")); + EXPECT_THAT(match(*I, Req), UnorderedElementsAre("a::y1", "a::y2", "b::y3")); } TEST(MemIndexTest, NoMatchNestedScopes) { - MemIndex I; - I.build(generateSymbols({"a::y1", "a::b::y2"})); + auto I = MemIndex::build( + generateSymbols({"a::y1", "a::b::y2"})); FuzzyFindRequest Req; Req.Query = "y"; Req.Scopes = {"a::"}; - EXPECT_THAT(match(I, Req), UnorderedElementsAre("a::y1")); + EXPECT_THAT(match(*I, Req), UnorderedElementsAre("a::y1")); } TEST(MemIndexTest, IgnoreCases) { - MemIndex I; - I.build(generateSymbols({"ns::ABC", "ns::abc"})); + auto I = MemIndex::build( + generateSymbols({"ns::ABC", "ns::abc"})); FuzzyFindRequest Req; Req.Query = "AB"; Req.Scopes = {"ns::"}; - EXPECT_THAT(match(I, Req), UnorderedElementsAre("ns::ABC", "ns::abc")); + EXPECT_THAT(match(*I, Req), UnorderedElementsAre("ns::ABC", "ns::abc")); } TEST(MemIndexTest, Lookup) { - MemIndex I; - I.build(generateSymbols({"ns::abc", "ns::xyz"})); - EXPECT_THAT(lookup(I, SymbolID("ns::abc")), UnorderedElementsAre("ns::abc")); - EXPECT_THAT(lookup(I, {SymbolID("ns::abc"), SymbolID("ns::xyz")}), + auto I = MemIndex::build( + generateSymbols({"ns::abc", "ns::xyz"})); + EXPECT_THAT(lookup(*I, SymbolID("ns::abc")), UnorderedElementsAre("ns::abc")); + EXPECT_THAT(lookup(*I, {SymbolID("ns::abc"), SymbolID("ns::xyz")}), UnorderedElementsAre("ns::abc", "ns::xyz")); - EXPECT_THAT(lookup(I, {SymbolID("ns::nonono"), SymbolID("ns::xyz")}), + EXPECT_THAT(lookup(*I, {SymbolID("ns::nonono"), SymbolID("ns::xyz")}), UnorderedElementsAre("ns::xyz")); - EXPECT_THAT(lookup(I, SymbolID("ns::nonono")), UnorderedElementsAre()); + EXPECT_THAT(lookup(*I, SymbolID("ns::nonono")), UnorderedElementsAre()); } TEST(MergeIndexTest, Lookup) { - MemIndex I, J; - I.build(generateSymbols({"ns::A", "ns::B"})); - J.build(generateSymbols({"ns::B", "ns::C"})); - EXPECT_THAT(lookup(*mergeIndex(&I, &J), SymbolID("ns::A")), + auto I = MemIndex::build(generateSymbols({"ns::A", "ns::B"})), + J = MemIndex::build(generateSymbols({"ns::B", "ns::C"})); + auto M = mergeIndex(I.get(), J.get()); + EXPECT_THAT(lookup(*M, SymbolID("ns::A")), UnorderedElementsAre("ns::A")); - EXPECT_THAT(lookup(*mergeIndex(&I, &J), SymbolID("ns::B")), + EXPECT_THAT(lookup(*M, SymbolID("ns::B")), UnorderedElementsAre("ns::B")); - EXPECT_THAT(lookup(*mergeIndex(&I, &J), SymbolID("ns::C")), + EXPECT_THAT(lookup(*M, SymbolID("ns::C")), UnorderedElementsAre("ns::C")); - EXPECT_THAT( - lookup(*mergeIndex(&I, &J), {SymbolID("ns::A"), SymbolID("ns::B")}), - UnorderedElementsAre("ns::A", "ns::B")); - EXPECT_THAT( - lookup(*mergeIndex(&I, &J), {SymbolID("ns::A"), SymbolID("ns::C")}), - UnorderedElementsAre("ns::A", "ns::C")); - EXPECT_THAT(lookup(*mergeIndex(&I, &J), SymbolID("ns::D")), + EXPECT_THAT(lookup(*M, {SymbolID("ns::A"), SymbolID("ns::B")}), + UnorderedElementsAre("ns::A", "ns::B")); + EXPECT_THAT(lookup(*M, {SymbolID("ns::A"), SymbolID("ns::C")}), + UnorderedElementsAre("ns::A", "ns::C")); + EXPECT_THAT(lookup(*M, SymbolID("ns::D")), UnorderedElementsAre()); - EXPECT_THAT(lookup(*mergeIndex(&I, &J), {}), UnorderedElementsAre()); + EXPECT_THAT(lookup(*M, {}), UnorderedElementsAre()); } TEST(MergeIndexTest, FuzzyFind) { - MemIndex I, J; - I.build(generateSymbols({"ns::A", "ns::B"})); - J.build(generateSymbols({"ns::B", "ns::C"})); + auto I = MemIndex::build(generateSymbols({"ns::A", "ns::B"})), + J = MemIndex::build(generateSymbols({"ns::B", "ns::C"})); FuzzyFindRequest Req; Req.Scopes = {"ns::"}; - EXPECT_THAT(match(*mergeIndex(&I, &J), Req), + EXPECT_THAT(match(*mergeIndex(I.get(), J.get()), Req), UnorderedElementsAre("ns::A", "ns::B", "ns::C")); } Index: unittests/clangd/FileIndexTests.cpp =================================================================== --- unittests/clangd/FileIndexTests.cpp +++ unittests/clangd/FileIndexTests.cpp @@ -39,41 +39,44 @@ return llvm::make_unique<SymbolSlab>(std::move(Slab).build()); } -std::vector<std::string> -getSymbolNames(const std::vector<const Symbol *> &Symbols) { +std::vector<std::string> getSymbolNames(const SymbolIndex &I, + std::string Query = "") { + FuzzyFindRequest Req; + Req.Query = Query; std::vector<std::string> Names; - for (const Symbol *Sym : Symbols) - Names.push_back(Sym->Name); + I.fuzzyFind(Req, [&](const Symbol& S) { + Names.push_back(S.Name); + }); return Names; } TEST(FileSymbolsTest, UpdateAndGet) { FileSymbols FS; - EXPECT_THAT(getSymbolNames(*FS.allSymbols()), UnorderedElementsAre()); + EXPECT_THAT(getSymbolNames(*FS.buildMemIndex()), UnorderedElementsAre()); FS.update("f1", numSlab(1, 3)); - EXPECT_THAT(getSymbolNames(*FS.allSymbols()), + EXPECT_THAT(getSymbolNames(*FS.buildMemIndex()), UnorderedElementsAre("1", "2", "3")); } TEST(FileSymbolsTest, Overlap) { FileSymbols FS; FS.update("f1", numSlab(1, 3)); FS.update("f2", numSlab(3, 5)); - EXPECT_THAT(getSymbolNames(*FS.allSymbols()), - UnorderedElementsAre("1", "2", "3", "3", "4", "5")); + EXPECT_THAT(getSymbolNames(*FS.buildMemIndex()), + UnorderedElementsAre("1", "2", "3", "4", "5")); } TEST(FileSymbolsTest, SnapshotAliveAfterRemove) { FileSymbols FS; FS.update("f1", numSlab(1, 3)); - auto Symbols = FS.allSymbols(); + auto Symbols = FS.buildMemIndex(); EXPECT_THAT(getSymbolNames(*Symbols), UnorderedElementsAre("1", "2", "3")); FS.update("f1", nullptr); - EXPECT_THAT(getSymbolNames(*FS.allSymbols()), UnorderedElementsAre()); + EXPECT_THAT(getSymbolNames(*FS.buildMemIndex()), UnorderedElementsAre()); EXPECT_THAT(getSymbolNames(*Symbols), UnorderedElementsAre("1", "2", "3")); } Index: unittests/clangd/DexIndexTests.cpp =================================================================== --- unittests/clangd/DexIndexTests.cpp +++ unittests/clangd/DexIndexTests.cpp @@ -23,6 +23,7 @@ using ::testing::ElementsAre; using ::testing::UnorderedElementsAre; +using namespace llvm; namespace clang { namespace clangd { @@ -408,171 +409,145 @@ } TEST(DexIndex, Lookup) { - DexIndex I; - I.build(generateSymbols({"ns::abc", "ns::xyz"})); - EXPECT_THAT(lookup(I, SymbolID("ns::abc")), UnorderedElementsAre("ns::abc")); - EXPECT_THAT(lookup(I, {SymbolID("ns::abc"), SymbolID("ns::xyz")}), + auto I = DexIndex::build(generateSymbols({"ns::abc", "ns::xyz"})); + EXPECT_THAT(lookup(*I, SymbolID("ns::abc")), UnorderedElementsAre("ns::abc")); + EXPECT_THAT(lookup(*I, {SymbolID("ns::abc"), SymbolID("ns::xyz")}), UnorderedElementsAre("ns::abc", "ns::xyz")); - EXPECT_THAT(lookup(I, {SymbolID("ns::nonono"), SymbolID("ns::xyz")}), + EXPECT_THAT(lookup(*I, {SymbolID("ns::nonono"), SymbolID("ns::xyz")}), UnorderedElementsAre("ns::xyz")); - EXPECT_THAT(lookup(I, SymbolID("ns::nonono")), UnorderedElementsAre()); + EXPECT_THAT(lookup(*I, SymbolID("ns::nonono")), UnorderedElementsAre()); } TEST(DexIndex, FuzzyFind) { - DexIndex Index; - Index.build(generateSymbols({"ns::ABC", "ns::BCD", "::ABC", "ns::nested::ABC", - "other::ABC", "other::A"})); + auto Index = DexIndex::build( + generateSymbols({"ns::ABC", "ns::BCD", "::ABC", "ns::nested::ABC", + "other::ABC", "other::A"})); FuzzyFindRequest Req; Req.Query = "ABC"; Req.Scopes = {"ns::"}; - EXPECT_THAT(match(Index, Req), UnorderedElementsAre("ns::ABC")); + EXPECT_THAT(match(*Index, Req), UnorderedElementsAre("ns::ABC")); Req.Scopes = {"ns::", "ns::nested::"}; - EXPECT_THAT(match(Index, Req), + EXPECT_THAT(match(*Index, Req), UnorderedElementsAre("ns::ABC", "ns::nested::ABC")); Req.Query = "A"; Req.Scopes = {"other::"}; - EXPECT_THAT(match(Index, Req), + EXPECT_THAT(match(*Index, Req), UnorderedElementsAre("other::A", "other::ABC")); Req.Query = ""; Req.Scopes = {}; - EXPECT_THAT(match(Index, Req), + EXPECT_THAT(match(*Index, Req), UnorderedElementsAre("ns::ABC", "ns::BCD", "::ABC", "ns::nested::ABC", "other::ABC", "other::A")); } TEST(DexIndexTest, FuzzyMatchQ) { - DexIndex I; - I.build( + auto I = DexIndex::build( generateSymbols({"LaughingOutLoud", "LionPopulation", "LittleOldLady"})); FuzzyFindRequest Req; Req.Query = "lol"; Req.MaxCandidateCount = 2; - EXPECT_THAT(match(I, Req), + EXPECT_THAT(match(*I, Req), UnorderedElementsAre("LaughingOutLoud", "LittleOldLady")); } -TEST(DexIndexTest, DexIndexSymbolsRecycled) { - DexIndex I; - std::weak_ptr<SlabAndPointers> Symbols; - I.build(generateNumSymbols(0, 10, &Symbols)); - FuzzyFindRequest Req; - Req.Query = "7"; - EXPECT_THAT(match(I, Req), UnorderedElementsAre("7")); - - EXPECT_FALSE(Symbols.expired()); - // Release old symbols. - I.build(generateNumSymbols(0, 0)); - EXPECT_TRUE(Symbols.expired()); -} - // FIXME(kbobyrev): This test is different for DexIndex and MemIndex: while // MemIndex manages response deduplication, DexIndex simply returns all matched // symbols which means there might be equivalent symbols in the response. // Before drop-in replacement of MemIndex with DexIndex happens, FileIndex // should handle deduplication instead. TEST(DexIndexTest, DexIndexDeduplicate) { - auto Symbols = generateNumSymbols(0, 10); - - // Inject duplicates. - auto Sym = symbol("7"); - Symbols->push_back(&Sym); - Symbols->push_back(&Sym); - Symbols->push_back(&Sym); - + std::vector<Symbol> Symbols = {symbol("1"), symbol("2"), symbol("3"), + symbol("2") /* duplicate */}; FuzzyFindRequest Req; - Req.Query = "7"; - DexIndex I; - I.build(std::move(Symbols)); - auto Matches = match(I, Req); - EXPECT_EQ(Matches.size(), 4u); + Req.Query = "2"; + DexIndex I(Symbols); + EXPECT_THAT(match(I, Req), ElementsAre("2","2")); } TEST(DexIndexTest, DexIndexLimitedNumMatches) { - DexIndex I; - I.build(generateNumSymbols(0, 100)); + auto I = DexIndex::build(generateNumSymbols(0, 100)); FuzzyFindRequest Req; Req.Query = "5"; Req.MaxCandidateCount = 3; bool Incomplete; - auto Matches = match(I, Req, &Incomplete); + auto Matches = match(*I, Req, &Incomplete); EXPECT_EQ(Matches.size(), Req.MaxCandidateCount); EXPECT_TRUE(Incomplete); } TEST(DexIndexTest, FuzzyMatch) { - DexIndex I; - I.build( + auto I = DexIndex::build( generateSymbols({"LaughingOutLoud", "LionPopulation", "LittleOldLady"})); FuzzyFindRequest Req; Req.Query = "lol"; Req.MaxCandidateCount = 2; - EXPECT_THAT(match(I, Req), + EXPECT_THAT(match(*I, Req), UnorderedElementsAre("LaughingOutLoud", "LittleOldLady")); } TEST(DexIndexTest, MatchQualifiedNamesWithoutSpecificScope) { - DexIndex I; - I.build(generateSymbols({"a::y1", "b::y2", "y3"})); + auto I = DexIndex::build( + generateSymbols({"a::y1", "b::y2", "y3"})); FuzzyFindRequest Req; Req.Query = "y"; - EXPECT_THAT(match(I, Req), UnorderedElementsAre("a::y1", "b::y2", "y3")); + EXPECT_THAT(match(*I, Req), UnorderedElementsAre("a::y1", "b::y2", "y3")); } TEST(DexIndexTest, MatchQualifiedNamesWithGlobalScope) { - DexIndex I; - I.build(generateSymbols({"a::y1", "b::y2", "y3"})); + auto I = DexIndex::build( + generateSymbols({"a::y1", "b::y2", "y3"})); FuzzyFindRequest Req; Req.Query = "y"; Req.Scopes = {""}; - EXPECT_THAT(match(I, Req), UnorderedElementsAre("y3")); + EXPECT_THAT(match(*I, Req), UnorderedElementsAre("y3")); } TEST(DexIndexTest, MatchQualifiedNamesWithOneScope) { - DexIndex I; - I.build(generateSymbols({"a::y1", "a::y2", "a::x", "b::y2", "y3"})); + auto I = DexIndex::build( + generateSymbols({"a::y1", "a::y2", "a::x", "b::y2", "y3"})); FuzzyFindRequest Req; Req.Query = "y"; Req.Scopes = {"a::"}; - EXPECT_THAT(match(I, Req), UnorderedElementsAre("a::y1", "a::y2")); + EXPECT_THAT(match(*I, Req), UnorderedElementsAre("a::y1", "a::y2")); } TEST(DexIndexTest, MatchQualifiedNamesWithMultipleScopes) { - DexIndex I; - I.build(generateSymbols({"a::y1", "a::y2", "a::x", "b::y3", "y3"})); + auto I = DexIndex::build( + generateSymbols({"a::y1", "a::y2", "a::x", "b::y3", "y3"})); FuzzyFindRequest Req; Req.Query = "y"; Req.Scopes = {"a::", "b::"}; - EXPECT_THAT(match(I, Req), UnorderedElementsAre("a::y1", "a::y2", "b::y3")); + EXPECT_THAT(match(*I, Req), UnorderedElementsAre("a::y1", "a::y2", "b::y3")); } TEST(DexIndexTest, NoMatchNestedScopes) { - DexIndex I; - I.build(generateSymbols({"a::y1", "a::b::y2"})); + auto I = DexIndex::build( + generateSymbols({"a::y1", "a::b::y2"})); FuzzyFindRequest Req; Req.Query = "y"; Req.Scopes = {"a::"}; - EXPECT_THAT(match(I, Req), UnorderedElementsAre("a::y1")); + EXPECT_THAT(match(*I, Req), UnorderedElementsAre("a::y1")); } TEST(DexIndexTest, IgnoreCases) { - DexIndex I; - I.build(generateSymbols({"ns::ABC", "ns::abc"})); + auto I = DexIndex::build( + generateSymbols({"ns::ABC", "ns::abc"})); FuzzyFindRequest Req; Req.Query = "AB"; Req.Scopes = {"ns::"}; - EXPECT_THAT(match(I, Req), UnorderedElementsAre("ns::ABC", "ns::abc")); + EXPECT_THAT(match(*I, Req), UnorderedElementsAre("ns::ABC", "ns::abc")); } TEST(DexIndexTest, Lookup) { - DexIndex I; - I.build(generateSymbols({"ns::abc", "ns::xyz"})); - EXPECT_THAT(lookup(I, SymbolID("ns::abc")), UnorderedElementsAre("ns::abc")); - EXPECT_THAT(lookup(I, {SymbolID("ns::abc"), SymbolID("ns::xyz")}), + auto I = DexIndex::build( + generateSymbols({"ns::abc", "ns::xyz"})); + EXPECT_THAT(lookup(*I, SymbolID("ns::abc")), UnorderedElementsAre("ns::abc")); + EXPECT_THAT(lookup(*I, {SymbolID("ns::abc"), SymbolID("ns::xyz")}), UnorderedElementsAre("ns::abc", "ns::xyz")); - EXPECT_THAT(lookup(I, {SymbolID("ns::nonono"), SymbolID("ns::xyz")}), + EXPECT_THAT(lookup(*I, {SymbolID("ns::nonono"), SymbolID("ns::xyz")}), UnorderedElementsAre("ns::xyz")); - EXPECT_THAT(lookup(I, SymbolID("ns::nonono")), UnorderedElementsAre()); + EXPECT_THAT(lookup(*I, SymbolID("ns::nonono")), UnorderedElementsAre()); } } // namespace Index: unittests/clangd/CodeCompleteTests.cpp =================================================================== --- unittests/clangd/CodeCompleteTests.cpp +++ unittests/clangd/CodeCompleteTests.cpp @@ -72,7 +72,7 @@ } MATCHER(IsDocumented, "") { return !arg.Documentation.empty(); } -std::unique_ptr<SymbolIndex> memIndex(std::vector<Symbol> Symbols) { +std::shared_ptr<SymbolIndex> memIndex(std::vector<Symbol> Symbols) { SymbolSlab::Builder Slab; for (const auto &Sym : Symbols) Slab.insert(Sym); @@ -83,7 +83,7 @@ Position point, std::vector<Symbol> IndexSymbols = {}, clangd::CodeCompleteOptions Opts = {}) { - std::unique_ptr<SymbolIndex> OverrideIndex; + std::shared_ptr<SymbolIndex> OverrideIndex; if (!IndexSymbols.empty()) { assert(!Opts.Index && "both Index and IndexSymbols given!"); OverrideIndex = memIndex(std::move(IndexSymbols)); @@ -99,7 +99,7 @@ CodeCompleteResult completions(ClangdServer &Server, StringRef Text, std::vector<Symbol> IndexSymbols = {}, clangd::CodeCompleteOptions Opts = {}) { - std::unique_ptr<SymbolIndex> OverrideIndex; + std::shared_ptr<SymbolIndex> OverrideIndex; if (!IndexSymbols.empty()) { assert(!Opts.Index && "both Index and IndexSymbols given!"); OverrideIndex = memIndex(std::move(IndexSymbols)); @@ -828,7 +828,7 @@ SignatureHelp signatures(StringRef Text, std::vector<Symbol> IndexSymbols = {}) { - std::unique_ptr<SymbolIndex> Index; + std::shared_ptr<SymbolIndex> Index; if (!IndexSymbols.empty()) Index = memIndex(IndexSymbols); Index: clangd/tool/ClangdMain.cpp =================================================================== --- clangd/tool/ClangdMain.cpp +++ clangd/tool/ClangdMain.cpp @@ -42,7 +42,7 @@ // Build an in-memory static index for global symbols from a YAML-format file. // The size of global symbols should be relatively small, so that all symbols // can be managed in memory. -std::unique_ptr<SymbolIndex> buildStaticIndex(llvm::StringRef YamlSymbolFile) { +std::shared_ptr<SymbolIndex> buildStaticIndex(llvm::StringRef YamlSymbolFile) { auto Buffer = llvm::MemoryBuffer::getFile(YamlSymbolFile); if (!Buffer) { llvm::errs() << "Can't open " << YamlSymbolFile << "\n"; @@ -296,7 +296,7 @@ if (!ResourceDir.empty()) Opts.ResourceDir = ResourceDir; Opts.BuildDynamicSymbolIndex = EnableIndex; - std::unique_ptr<SymbolIndex> StaticIdx; + std::shared_ptr<SymbolIndex> StaticIdx; if (EnableIndex && !YamlSymbolFile.empty()) { StaticIdx = buildStaticIndex(YamlSymbolFile); Opts.StaticIndex = StaticIdx.get(); Index: clangd/index/dex/DexIndex.h =================================================================== --- clangd/index/dex/DexIndex.h +++ clangd/index/dex/DexIndex.h @@ -25,7 +25,6 @@ #include "Iterator.h" #include "Token.h" #include "Trigram.h" -#include <mutex> namespace clang { namespace clangd { @@ -39,12 +38,15 @@ // 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<std::vector<const Symbol *>> Symbols); + // All symbols must outlive this index. + template <typename Range> DexIndex(Range &&Symbols) { + for (auto &&Sym : Symbols) + this->Symbols.push_back(&Sym); + buildIndex(); + } - /// \brief Build index from a symbol slab. - static std::unique_ptr<SymbolIndex> build(SymbolSlab Slab); + /// Builds an index from a slab. The shared_ptr manages the slab's lifetime. + static std::shared_ptr<SymbolIndex> build(SymbolSlab Slab); bool fuzzyFind(const FuzzyFindRequest &Req, @@ -60,19 +62,18 @@ size_t estimateMemoryUsage() const override; private: + void buildIndex(); - mutable std::mutex Mutex; - - std::shared_ptr<std::vector<const Symbol *>> Symbols /*GUARDED_BY(Mutex)*/; - llvm::DenseMap<SymbolID, const Symbol *> LookupTable /*GUARDED_BY(Mutex)*/; - llvm::DenseMap<const Symbol *, float> SymbolQuality /*GUARDED_BY(Mutex)*/; + std::vector<const Symbol *> Symbols; + llvm::DenseMap<SymbolID, const Symbol *> LookupTable; + llvm::DenseMap<const Symbol *, float> 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<Token, PostingList> InvertedIndex /*GUARDED_BY(Mutex)*/; + llvm::DenseMap<Token, PostingList> InvertedIndex; }; } // namespace dex Index: clangd/index/dex/DexIndex.cpp =================================================================== --- clangd/index/dex/DexIndex.cpp +++ clangd/index/dex/DexIndex.cpp @@ -36,46 +36,40 @@ } // namespace -void DexIndex::build(std::shared_ptr<std::vector<const Symbol *>> Syms) { - llvm::DenseMap<SymbolID, const Symbol *> TempLookupTable; - llvm::DenseMap<const Symbol *, float> 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<Token, PostingList> 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 &Token : generateSearchTokens(*Sym)) - TempInvertedIndex[Token].push_back(SymbolRank); - } - - { - std::lock_guard<std::mutex> Lock(Mutex); - - // Replace outdated index with the new one. - LookupTable = std::move(TempLookupTable); - Symbols = std::move(Syms); - InvertedIndex = std::move(TempInvertedIndex); - SymbolQuality = std::move(TempSymbolQuality); + InvertedIndex[Token].push_back(SymbolRank); } vlog("Built DexIndex with estimated memory usage {0} bytes.", estimateMemoryUsage()); } -std::unique_ptr<SymbolIndex> DexIndex::build(SymbolSlab Slab) { - auto Idx = llvm::make_unique<DexIndex>(); - Idx->build(getSymbolsFromSlab(std::move(Slab))); - return std::move(Idx); +std::shared_ptr<SymbolIndex> DexIndex::build(SymbolSlab Slab) { + struct IndexAndSlab { + IndexAndSlab(SymbolSlab Slab) + : Index(llvm::make_range(Slab.begin(), Slab.end())), + Slab(std::move(Slab)) {} + DexIndex Index; + SymbolSlab Slab; + }; + auto Storage = std::make_shared<IndexAndSlab>(std::move(Slab)); + return std::shared_ptr<SymbolIndex>(std::move(Storage), &Storage->Index); } /// Constructs iterators over tokens extracted from the query and exhausts it @@ -92,75 +86,69 @@ std::vector<std::unique_ptr<Iterator>> TopLevelChildren; const auto TrigramTokens = generateIdentifierTrigrams(Req.Query); - { - std::lock_guard<std::mutex> Lock(Mutex); - - // Generate query trigrams and construct AND iterator over all query - // trigrams. - std::vector<std::unique_ptr<Iterator>> TrigramIterators; - for (const auto &Trigram : TrigramTokens) { - const auto It = InvertedIndex.find(Trigram); - if (It != InvertedIndex.end()) - TrigramIterators.push_back(create(It->second)); - } - if (!TrigramIterators.empty()) - TopLevelChildren.push_back(createAnd(move(TrigramIterators))); - - // Generate scope tokens for search query. - std::vector<std::unique_ptr<Iterator>> ScopeIterators; - for (const auto &Scope : Req.Scopes) { - const auto It = InvertedIndex.find(Token(Token::Kind::Scope, Scope)); - if (It != InvertedIndex.end()) - ScopeIterators.push_back(create(It->second)); - } - // Add OR iterator for scopes if there are any Scope Iterators. - if (!ScopeIterators.empty()) - TopLevelChildren.push_back(createOr(move(ScopeIterators))); - - // Use TRUE iterator if both trigrams and scopes from the query are not - // present in the symbol index. - auto QueryIterator = TopLevelChildren.empty() - ? createTrue(Symbols->size()) - : createAnd(move(TopLevelChildren)); - // Retrieve more items than it was requested: some of the items with high - // final score might not be retrieved otherwise. - // FIXME(kbobyrev): Pre-scoring retrieval threshold should be adjusted as - // using 100x of the requested number might not be good in practice, e.g. - // when the requested number of items is small. - const unsigned ItemsToRetrieve = 100 * Req.MaxCandidateCount; - auto Root = createLimit(move(QueryIterator), ItemsToRetrieve); - // FIXME(kbobyrev): Add boosting to the query and utilize retrieved - // boosting scores. - std::vector<std::pair<DocID, float>> SymbolDocIDs = consume(*Root); - - // Retrieve top Req.MaxCandidateCount items. - std::priority_queue<std::pair<float, const Symbol *>> Top; - for (const auto &P : SymbolDocIDs) { - const DocID SymbolDocID = P.first; - const auto *Sym = (*Symbols)[SymbolDocID]; - const llvm::Optional<float> Score = Filter.match(Sym->Name); - if (!Score) - continue; - // Multiply score by a negative factor so that Top stores items with the - // highest actual score. - Top.emplace(-(*Score) * SymbolQuality.find(Sym)->second, Sym); - if (Top.size() > Req.MaxCandidateCount) { - More = true; - Top.pop(); - } + // Generate query trigrams and construct AND iterator over all query + // trigrams. + std::vector<std::unique_ptr<Iterator>> TrigramIterators; + for (const auto &Trigram : TrigramTokens) { + const auto It = InvertedIndex.find(Trigram); + if (It != InvertedIndex.end()) + TrigramIterators.push_back(create(It->second)); + } + if (!TrigramIterators.empty()) + TopLevelChildren.push_back(createAnd(move(TrigramIterators))); + + // Generate scope tokens for search query. + std::vector<std::unique_ptr<Iterator>> ScopeIterators; + for (const auto &Scope : Req.Scopes) { + const auto It = InvertedIndex.find(Token(Token::Kind::Scope, Scope)); + if (It != InvertedIndex.end()) + ScopeIterators.push_back(create(It->second)); + } + // Add OR iterator for scopes if there are any Scope Iterators. + if (!ScopeIterators.empty()) + TopLevelChildren.push_back(createOr(move(ScopeIterators))); + + // Use TRUE iterator if both trigrams and scopes from the query are not + // present in the symbol index. + auto QueryIterator = TopLevelChildren.empty() + ? createTrue(Symbols.size()) + : createAnd(move(TopLevelChildren)); + // Retrieve more items than it was requested: some of the items with high + // final score might not be retrieved otherwise. + // FIXME(kbobyrev): Pre-scoring retrieval threshold should be adjusted as + // using 100x of the requested number might not be good in practice, e.g. + // when the requested number of items is small. + const unsigned ItemsToRetrieve = 100 * Req.MaxCandidateCount; + auto Root = createLimit(move(QueryIterator), ItemsToRetrieve); + // FIXME(kbobyrev): Add boosting to the query and utilize retrieved + // boosting scores. + std::vector<std::pair<DocID, float>> SymbolDocIDs = consume(*Root); + + // Retrieve top Req.MaxCandidateCount items. + std::priority_queue<std::pair<float, const Symbol *>> Top; + for (const auto &P : SymbolDocIDs) { + const DocID SymbolDocID = P.first; + const auto *Sym = Symbols[SymbolDocID]; + const llvm::Optional<float> Score = Filter.match(Sym->Name); + if (!Score) + continue; + // Multiply score by a negative factor so that Top stores items with the + // highest actual score. + Top.emplace(-(*Score) * SymbolQuality.find(Sym)->second, Sym); + if (Top.size() > Req.MaxCandidateCount) { + More = true; + Top.pop(); } - - // Apply callback to the top Req.MaxCandidateCount items. - for (; !Top.empty(); Top.pop()) - Callback(*Top.top().second); } + // Apply callback to the top Req.MaxCandidateCount items. + for (; !Top.empty(); Top.pop()) + Callback(*Top.top().second); return More; } void DexIndex::lookup(const LookupRequest &Req, llvm::function_ref<void(const Symbol &)> Callback) const { - std::lock_guard<std::mutex> Lock(Mutex); for (const auto &ID : Req.IDs) { auto I = LookupTable.find(ID); if (I != LookupTable.end()) @@ -175,8 +163,6 @@ } size_t DexIndex::estimateMemoryUsage() const { - std::lock_guard<std::mutex> Lock(Mutex); - size_t Bytes = LookupTable.size() * sizeof(std::pair<SymbolID, const Symbol *>); Bytes += SymbolQuality.size() * sizeof(std::pair<const Symbol *, float>); Index: clangd/index/MemIndex.h =================================================================== --- clangd/index/MemIndex.h +++ clangd/index/MemIndex.h @@ -16,16 +16,18 @@ namespace clang { namespace clangd { -/// \brief This implements an index for a (relatively small) set of symbols that -/// can be easily managed in memory. +/// MemIndex is a naive in-memory index suitable for a small set of symbols. class MemIndex : 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<std::vector<const Symbol *>> Symbols); + // All symbols must outlive this index. + template <typename Range> MemIndex(Range &&Symbols) { + for (const Symbol &S : Symbols) + Index[S.ID] = &S; + } + MemIndex() = default; - /// \brief Build index from a symbol slab. - static std::unique_ptr<SymbolIndex> build(SymbolSlab Slab); + /// Builds an index from a slab. The shared_ptr manages the slab's lifetime. + static std::shared_ptr<SymbolIndex> build(SymbolSlab Slab); bool fuzzyFind(const FuzzyFindRequest &Req, @@ -42,19 +44,10 @@ size_t estimateMemoryUsage() const override; private: - - std::shared_ptr<std::vector<const Symbol *>> Symbols; // Index is a set of symbols that are deduplicated by symbol IDs. - // FIXME: build smarter index structure. llvm::DenseMap<SymbolID, const Symbol *> Index; - mutable std::mutex Mutex; }; -// Returns pointers to the symbols in given slab and bundles slab lifetime with -// returned symbol pointers so that the pointers are never invalid. -std::shared_ptr<std::vector<const Symbol *>> -getSymbolsFromSlab(SymbolSlab Slab); - } // namespace clangd } // namespace clang Index: clangd/index/MemIndex.cpp =================================================================== --- clangd/index/MemIndex.cpp +++ clangd/index/MemIndex.cpp @@ -15,26 +15,16 @@ namespace clang { namespace clangd { -void MemIndex::build(std::shared_ptr<std::vector<const Symbol *>> Syms) { - llvm::DenseMap<SymbolID, const Symbol *> TempIndex; - for (const Symbol *Sym : *Syms) - TempIndex[Sym->ID] = Sym; - - // Swap out the old symbols and index. - { - std::lock_guard<std::mutex> Lock(Mutex); - Index = std::move(TempIndex); - Symbols = std::move(Syms); // Relase old symbols. - } - - vlog("Built MemIndex with estimated memory usage {0} bytes.", - estimateMemoryUsage()); -} - -std::unique_ptr<SymbolIndex> MemIndex::build(SymbolSlab Slab) { - auto Idx = llvm::make_unique<MemIndex>(); - Idx->build(getSymbolsFromSlab(std::move(Slab))); - return std::move(Idx); +std::shared_ptr<SymbolIndex> MemIndex::build(SymbolSlab Slab) { + struct SlabAndIndex { + SlabAndIndex(SymbolSlab Slab) + : Index(llvm::make_range(Slab.begin(), Slab.end())), + Slab(std::move(Slab)) {} + MemIndex Index; + SymbolSlab Slab; + }; + auto Payload = std::make_shared<SlabAndIndex>(std::move(Slab)); + return std::shared_ptr<SymbolIndex>(std::move(Payload), &Payload->Index); } bool MemIndex::fuzzyFind( @@ -46,34 +36,30 @@ std::priority_queue<std::pair<float, const Symbol *>> Top; FuzzyMatcher Filter(Req.Query); bool More = false; - { - std::lock_guard<std::mutex> Lock(Mutex); - for (const auto Pair : Index) { - const Symbol *Sym = Pair.second; + for (const auto Pair : Index) { + const Symbol *Sym = Pair.second; - // Exact match against all possible scopes. - if (!Req.Scopes.empty() && !llvm::is_contained(Req.Scopes, Sym->Scope)) - continue; - if (Req.RestrictForCodeCompletion && !Sym->IsIndexedForCodeCompletion) - continue; + // Exact match against all possible scopes. + if (!Req.Scopes.empty() && !llvm::is_contained(Req.Scopes, Sym->Scope)) + continue; + if (Req.RestrictForCodeCompletion && !Sym->IsIndexedForCodeCompletion) + continue; - if (auto Score = Filter.match(Sym->Name)) { - Top.emplace(-*Score * quality(*Sym), Sym); - if (Top.size() > Req.MaxCandidateCount) { - More = true; - Top.pop(); - } + if (auto Score = Filter.match(Sym->Name)) { + Top.emplace(-*Score * quality(*Sym), Sym); + if (Top.size() > Req.MaxCandidateCount) { + More = true; + Top.pop(); } } - for (; !Top.empty(); Top.pop()) - Callback(*Top.top().second); } + for (; !Top.empty(); Top.pop()) + Callback(*Top.top().second); return More; } void MemIndex::lookup(const LookupRequest &Req, llvm::function_ref<void(const Symbol &)> Callback) const { - std::lock_guard<std::mutex> Lock(Mutex); for (const auto &ID : Req.IDs) { auto I = Index.find(ID); if (I != Index.end()) @@ -87,22 +73,7 @@ log("findOccurrences is not implemented."); } -std::shared_ptr<std::vector<const Symbol *>> -getSymbolsFromSlab(SymbolSlab Slab) { - struct Snapshot { - SymbolSlab Slab; - std::vector<const Symbol *> Pointers; - }; - auto Snap = std::make_shared<Snapshot>(); - Snap->Slab = std::move(Slab); - for (auto &Sym : Snap->Slab) - Snap->Pointers.push_back(&Sym); - return std::shared_ptr<std::vector<const Symbol *>>(std::move(Snap), - &Snap->Pointers); -} - size_t MemIndex::estimateMemoryUsage() const { - std::lock_guard<std::mutex> Lock(Mutex); return Index.getMemorySize(); } Index: clangd/index/Index.h =================================================================== --- clangd/index/Index.h +++ clangd/index/Index.h @@ -19,6 +19,7 @@ #include "llvm/ADT/StringExtras.h" #include "llvm/Support/StringSaver.h" #include <array> +#include <mutex> #include <string> #include <tuple> @@ -363,7 +364,7 @@ SymbolOccurrenceKind Filter; }; -/// \brief Interface for symbol indexes that can be used for searching or +/// Interface for symbol indexes that can be used for searching or /// matching symbols among a set of symbols based on names or unique IDs. class SymbolIndex { public: @@ -402,6 +403,31 @@ virtual size_t estimateMemoryUsage() const = 0; }; +// Delegating implementation of SymbolIndex whose delegate can be swapped out. +class SwapIndex : public SymbolIndex { +public: + // If an index is not provided, reset() must be called. + SwapIndex(std::shared_ptr<SymbolIndex> Index = nullptr) + : Index(std::move(Index)) {} + void reset(std::shared_ptr<SymbolIndex>); + + // SymbolIndex methods delegate to the current index, which is kept alive + // until the call returns (even if reset() is called). + bool fuzzyFind(const FuzzyFindRequest &, + llvm::function_ref<void(const Symbol &)>) const override; + void lookup(const LookupRequest &, + llvm::function_ref<void(const Symbol &)>) const override; + void findOccurrences( + const OccurrencesRequest &, + llvm::function_ref<void(const SymbolOccurrence &)>) const override; + size_t estimateMemoryUsage() const override; + +private: + std::shared_ptr<SymbolIndex> snapshot() const; + mutable std::mutex Mutex; + std::shared_ptr<SymbolIndex> Index; +}; + } // namespace clangd } // namespace clang Index: clangd/index/Index.cpp =================================================================== --- clangd/index/Index.cpp +++ clangd/index/Index.cpp @@ -128,5 +128,36 @@ return SymbolSlab(std::move(NewArena), std::move(Symbols)); } +void SwapIndex::reset(std::shared_ptr<SymbolIndex> Index) { + // Keep the old index alive, so we don't destroy it under lock (may be slow). + std::shared_ptr<SymbolIndex> Pin; + { + std::lock_guard<std::mutex> Lock(Mutex); + Pin = std::move(this->Index); + this->Index = std::move(Index); + } +} +std::shared_ptr<SymbolIndex> SwapIndex::snapshot() const { + std::lock_guard<std::mutex> Lock(Mutex); + return Index; +} + +bool SwapIndex::fuzzyFind(const FuzzyFindRequest &R, + llvm::function_ref<void(const Symbol &)> CB) const { + return snapshot()->fuzzyFind(R, CB); +} +void SwapIndex::lookup(const LookupRequest &R, + llvm::function_ref<void(const Symbol &)> CB) const { + return snapshot()->lookup(R, CB); +} +void SwapIndex::findOccurrences( + const OccurrencesRequest &R, + llvm::function_ref<void(const SymbolOccurrence &)> CB) const { + return snapshot()->findOccurrences(R, CB); +} +size_t SwapIndex::estimateMemoryUsage() const { + return snapshot()->estimateMemoryUsage(); +} + } // namespace clangd } // namespace clang Index: clangd/index/FileIndex.h =================================================================== --- clangd/index/FileIndex.h +++ clangd/index/FileIndex.h @@ -24,7 +24,7 @@ namespace clang { namespace clangd { -/// \brief A container of Symbols from several source files. It can be updated +/// A container of Symbols from several source files. It can be updated /// at source-file granularity, replacing all symbols from one file with a new /// set. /// @@ -39,28 +39,28 @@ /// locking when we swap or obtain refereces to snapshots. class FileSymbols { public: - /// \brief Updates all symbols in a file. If \p Slab is nullptr, symbols for + /// Updates all symbols in a file. If \p Slab is nullptr, symbols for /// \p Path will be removed. void update(PathRef Path, std::unique_ptr<SymbolSlab> Slab); - // The shared_ptr keeps the symbols alive - std::shared_ptr<std::vector<const Symbol *>> allSymbols(); + // The shared_ptr keeps the symbols alive. + std::shared_ptr<SymbolIndex> buildMemIndex(); private: mutable std::mutex Mutex; - /// \brief Stores the latest snapshots for all active files. + /// Stores the latest snapshots for all active files. llvm::StringMap<std::shared_ptr<SymbolSlab>> FileToSlabs; }; -/// \brief This manages symbls from files and an in-memory index on all symbols. -class FileIndex : public SymbolIndex { +/// This manages symbols from files and an in-memory index on all symbols. +class FileIndex : public SwapIndex { public: /// If URISchemes is empty, the default schemes in SymbolCollector will be /// used. FileIndex(std::vector<std::string> URISchemes = {}); - /// \brief Update symbols in \p Path with symbols in \p AST. If \p AST is + /// Update symbols in \p Path with symbols in \p AST. If \p AST is /// nullptr, this removes all symbols in the file. /// If \p AST is not null, \p PP cannot be null and it should be the /// preprocessor that was used to build \p AST. @@ -70,23 +70,11 @@ update(PathRef Path, ASTContext *AST, std::shared_ptr<Preprocessor> PP, llvm::Optional<llvm::ArrayRef<Decl *>> TopLevelDecls = llvm::None); - bool - fuzzyFind(const FuzzyFindRequest &Req, - llvm::function_ref<void(const Symbol &)> Callback) const override; - - void lookup(const LookupRequest &Req, - llvm::function_ref<void(const Symbol &)> Callback) const override; - - - void findOccurrences(const OccurrencesRequest &Req, - llvm::function_ref<void(const SymbolOccurrence &)> - Callback) const override; - - size_t estimateMemoryUsage() const override; - private: + // Only update() should swap the index. + using SwapIndex::reset; + FileSymbols FSymbols; - MemIndex Index; std::vector<std::string> URISchemes; }; Index: clangd/index/FileIndex.cpp =================================================================== --- clangd/index/FileIndex.cpp +++ clangd/index/FileIndex.cpp @@ -52,7 +52,9 @@ } FileIndex::FileIndex(std::vector<std::string> URISchemes) - : URISchemes(std::move(URISchemes)) {} + : URISchemes(std::move(URISchemes)) { + reset(FSymbols.buildMemIndex()); +} void FileSymbols::update(PathRef Path, std::unique_ptr<SymbolSlab> Slab) { std::lock_guard<std::mutex> Lock(Mutex); @@ -62,27 +64,27 @@ FileToSlabs[Path] = std::move(Slab); } -std::shared_ptr<std::vector<const Symbol *>> FileSymbols::allSymbols() { +std::shared_ptr<SymbolIndex> FileSymbols::buildMemIndex() { // The snapshot manages life time of symbol slabs and provides pointers of all // symbols in all slabs. struct Snapshot { - std::vector<const Symbol *> Pointers; - std::vector<std::shared_ptr<SymbolSlab>> KeepAlive; + llvm::Optional<MemIndex> Index; + std::vector<std::shared_ptr<SymbolSlab>> Slabs; }; auto Snap = std::make_shared<Snapshot>(); { std::lock_guard<std::mutex> Lock(Mutex); - - for (const auto &FileAndSlab : FileToSlabs) { - Snap->KeepAlive.push_back(FileAndSlab.second); - for (const auto &Iter : *FileAndSlab.second) - Snap->Pointers.push_back(&Iter); - } + for (const auto &FileAndSlab : FileToSlabs) + Snap->Slabs.push_back(FileAndSlab.second); } - auto *Pointers = &Snap->Pointers; - // Use aliasing constructor to keep the snapshot alive along with the - // pointers. - return {std::move(Snap), Pointers}; + std::vector<const Symbol*> AllSymbols; + for (const auto &Slab : Snap->Slabs) + for (const auto &Sym : *Slab) + AllSymbols.push_back(&Sym); + Snap->Index.emplace(llvm::make_pointee_range(AllSymbols)); + // Use aliasing constructor to keep the slabs alive along with the index. + return std::shared_ptr<SymbolIndex>(std::move(Snap), + Snap->Index.getPointer()); } void FileIndex::update(PathRef Path, ASTContext *AST, @@ -96,30 +98,7 @@ *Slab = indexAST(*AST, PP, TopLevelDecls, URISchemes); FSymbols.update(Path, std::move(Slab)); } - auto Symbols = FSymbols.allSymbols(); - Index.build(std::move(Symbols)); -} - -bool FileIndex::fuzzyFind( - const FuzzyFindRequest &Req, - llvm::function_ref<void(const Symbol &)> Callback) const { - return Index.fuzzyFind(Req, Callback); -} - -void FileIndex::lookup( - const LookupRequest &Req, - llvm::function_ref<void(const Symbol &)> Callback) const { - Index.lookup(Req, Callback); -} - -void FileIndex::findOccurrences( - const OccurrencesRequest &Req, - llvm::function_ref<void(const SymbolOccurrence &)> Callback) const { - log("findOccurrences is not implemented."); -} - -size_t FileIndex::estimateMemoryUsage() const { - return Index.estimateMemoryUsage(); + reset(FSymbols.buildMemIndex()); } } // namespace clangd
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits