Skip to content

Commit 7067454

Browse files
kadircetkadircet
committedMay 9, 2019
[clangd] Count number of references while merging RefSlabs inside FileIndex
Summary: For counting number of references clangd was relying on merging every duplication of a symbol. Unfortunately this does not apply to FileIndex(and one of its users' BackgroundIndex), since we get rid of duplication by simply dropping symbols coming from non-canonical locations. So only one or two(coming from canonical declaration header and defined source file, if exists) replications of the same symbol reaches merging step. This patch changes reference counting logic to rather count number of different RefSlabs a given SymbolID exists. Reviewers: ilya-biryukov Subscribers: ioeric, MaskRay, jkorous, mgrang, arphaman, jdoerfert, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D59481 llvm-svn: 360344
·
llvmorg-21-initllvmorg-9.0.0-rc1
1 parent 82e68f5 commit 7067454

File tree

7 files changed

+104
-33
lines changed

7 files changed

+104
-33
lines changed
 

‎clang-tools-extra/clangd/index/Background.cpp

+7-3
Original file line numberDiff line numberDiff line change
@@ -356,7 +356,8 @@ void BackgroundIndex::update(llvm::StringRef MainFile, IndexFileIn Index,
356356
// This can override a newer version that is added in another thread, if
357357
// this thread sees the older version but finishes later. This should be
358358
// rare in practice.
359-
IndexedSymbols.update(Path, std::move(SS), std::move(RS));
359+
IndexedSymbols.update(Path, std::move(SS), std::move(RS),
360+
Path == MainFile);
360361
}
361362
}
362363
}
@@ -478,7 +479,8 @@ BackgroundIndex::loadShard(const tooling::CompileCommand &Cmd,
478479
struct ShardInfo {
479480
std::string AbsolutePath;
480481
std::unique_ptr<IndexFileIn> Shard;
481-
FileDigest Digest;
482+
FileDigest Digest = {};
483+
bool CountReferences = false;
482484
};
483485
std::vector<ShardInfo> IntermediateSymbols;
484486
// Make sure we don't have duplicate elements in the queue. Keys are absolute
@@ -539,6 +541,7 @@ BackgroundIndex::loadShard(const tooling::CompileCommand &Cmd,
539541
SI.AbsolutePath = CurDependency.Path;
540542
SI.Shard = std::move(Shard);
541543
SI.Digest = I.getValue().Digest;
544+
SI.CountReferences = I.getValue().IsTU;
542545
IntermediateSymbols.push_back(std::move(SI));
543546
// Check if the source needs re-indexing.
544547
// Get the digest, skip it if file doesn't exist.
@@ -568,7 +571,8 @@ BackgroundIndex::loadShard(const tooling::CompileCommand &Cmd,
568571
? llvm::make_unique<RefSlab>(std::move(*SI.Shard->Refs))
569572
: nullptr;
570573
IndexedFileDigests[SI.AbsolutePath] = SI.Digest;
571-
IndexedSymbols.update(SI.AbsolutePath, std::move(SS), std::move(RS));
574+
IndexedSymbols.update(SI.AbsolutePath, std::move(SS), std::move(RS),
575+
SI.CountReferences);
572576
}
573577
}
574578

‎clang-tools-extra/clangd/index/FileIndex.cpp

+31-12
Original file line numberDiff line numberDiff line change
@@ -90,28 +90,36 @@ SymbolSlab indexHeaderSymbols(ASTContext &AST, std::shared_ptr<Preprocessor> PP,
9090
}
9191

9292
void FileSymbols::update(PathRef Path, std::unique_ptr<SymbolSlab> Symbols,
93-
std::unique_ptr<RefSlab> Refs) {
93+
std::unique_ptr<RefSlab> Refs, bool CountReferences) {
9494
std::lock_guard<std::mutex> Lock(Mutex);
9595
if (!Symbols)
9696
FileToSymbols.erase(Path);
9797
else
9898
FileToSymbols[Path] = std::move(Symbols);
99-
if (!Refs)
99+
if (!Refs) {
100100
FileToRefs.erase(Path);
101-
else
102-
FileToRefs[Path] = std::move(Refs);
101+
return;
102+
}
103+
RefSlabAndCountReferences Item;
104+
Item.CountReferences = CountReferences;
105+
Item.Slab = std::move(Refs);
106+
FileToRefs[Path] = std::move(Item);
103107
}
104108

105109
std::unique_ptr<SymbolIndex>
106110
FileSymbols::buildIndex(IndexType Type, DuplicateHandling DuplicateHandle) {
107111
std::vector<std::shared_ptr<SymbolSlab>> SymbolSlabs;
108112
std::vector<std::shared_ptr<RefSlab>> RefSlabs;
113+
std::vector<RefSlab *> MainFileRefs;
109114
{
110115
std::lock_guard<std::mutex> Lock(Mutex);
111116
for (const auto &FileAndSymbols : FileToSymbols)
112117
SymbolSlabs.push_back(FileAndSymbols.second);
113-
for (const auto &FileAndRefs : FileToRefs)
114-
RefSlabs.push_back(FileAndRefs.second);
118+
for (const auto &FileAndRefs : FileToRefs) {
119+
RefSlabs.push_back(FileAndRefs.second.Slab);
120+
if (FileAndRefs.second.CountReferences)
121+
MainFileRefs.push_back(RefSlabs.back().get());
122+
}
115123
}
116124
std::vector<const Symbol *> AllSymbols;
117125
std::vector<Symbol> SymsStorage;
@@ -120,25 +128,35 @@ FileSymbols::buildIndex(IndexType Type, DuplicateHandling DuplicateHandle) {
120128
llvm::DenseMap<SymbolID, Symbol> Merged;
121129
for (const auto &Slab : SymbolSlabs) {
122130
for (const auto &Sym : *Slab) {
131+
assert(Sym.References == 0 &&
132+
"Symbol with non-zero references sent to FileSymbols");
123133
auto I = Merged.try_emplace(Sym.ID, Sym);
124134
if (!I.second)
125135
I.first->second = mergeSymbol(I.first->second, Sym);
126136
}
127137
}
138+
for (const RefSlab *Refs : MainFileRefs)
139+
for (const auto &Sym : *Refs) {
140+
auto It = Merged.find(Sym.first);
141+
assert(It != Merged.end() && "Reference to unknown symbol");
142+
It->getSecond().References += Sym.second.size();
143+
}
128144
SymsStorage.reserve(Merged.size());
129145
for (auto &Sym : Merged) {
130146
SymsStorage.push_back(std::move(Sym.second));
131147
AllSymbols.push_back(&SymsStorage.back());
132148
}
133-
// FIXME: aggregate symbol reference count based on references.
134149
break;
135150
}
136151
case DuplicateHandling::PickOne: {
137152
llvm::DenseSet<SymbolID> AddedSymbols;
138153
for (const auto &Slab : SymbolSlabs)
139-
for (const auto &Sym : *Slab)
154+
for (const auto &Sym : *Slab) {
155+
assert(Sym.References == 0 &&
156+
"Symbol with non-zero references sent to FileSymbols");
140157
if (AddedSymbols.insert(Sym.ID).second)
141158
AllSymbols.push_back(&Sym);
159+
}
142160
break;
143161
}
144162
}
@@ -201,9 +219,9 @@ void FileIndex::updatePreamble(PathRef Path, ASTContext &AST,
201219
std::shared_ptr<Preprocessor> PP,
202220
const CanonicalIncludes &Includes) {
203221
auto Symbols = indexHeaderSymbols(AST, std::move(PP), Includes);
204-
PreambleSymbols.update(Path,
205-
llvm::make_unique<SymbolSlab>(std::move(Symbols)),
206-
llvm::make_unique<RefSlab>());
222+
PreambleSymbols.update(
223+
Path, llvm::make_unique<SymbolSlab>(std::move(Symbols)),
224+
llvm::make_unique<RefSlab>(), /*CountReferences=*/false);
207225
PreambleIndex.reset(
208226
PreambleSymbols.buildIndex(UseDex ? IndexType::Heavy : IndexType::Light,
209227
DuplicateHandling::PickOne));
@@ -213,7 +231,8 @@ void FileIndex::updateMain(PathRef Path, ParsedAST &AST) {
213231
auto Contents = indexMainDecls(AST);
214232
MainFileSymbols.update(
215233
Path, llvm::make_unique<SymbolSlab>(std::move(Contents.first)),
216-
llvm::make_unique<RefSlab>(std::move(Contents.second)));
234+
llvm::make_unique<RefSlab>(std::move(Contents.second)),
235+
/*CountReferences=*/true);
217236
MainFileIndex.reset(
218237
MainFileSymbols.buildIndex(IndexType::Light, DuplicateHandling::PickOne));
219238
}

‎clang-tools-extra/clangd/index/FileIndex.h

+11-3
Original file line numberDiff line numberDiff line change
@@ -60,21 +60,29 @@ class FileSymbols {
6060
public:
6161
/// Updates all symbols and refs in a file.
6262
/// If either is nullptr, corresponding data for \p Path will be removed.
63+
/// If CountReferences is true, \p Refs will be used for counting References
64+
/// during merging.
6365
void update(PathRef Path, std::unique_ptr<SymbolSlab> Slab,
64-
std::unique_ptr<RefSlab> Refs);
66+
std::unique_ptr<RefSlab> Refs, bool CountReferences);
6567

66-
// The index keeps the symbols alive.
68+
/// The index keeps the symbols alive.
69+
/// Will count Symbol::References based on number of references in the main
70+
/// files, while building the index with DuplicateHandling::Merge option.
6771
std::unique_ptr<SymbolIndex>
6872
buildIndex(IndexType,
6973
DuplicateHandling DuplicateHandle = DuplicateHandling::PickOne);
7074

7175
private:
76+
struct RefSlabAndCountReferences {
77+
std::shared_ptr<RefSlab> Slab;
78+
bool CountReferences = false;
79+
};
7280
mutable std::mutex Mutex;
7381

7482
/// Stores the latest symbol snapshots for all active files.
7583
llvm::StringMap<std::shared_ptr<SymbolSlab>> FileToSymbols;
7684
/// Stores the latest ref snapshots for all active files.
77-
llvm::StringMap<std::shared_ptr<RefSlab>> FileToRefs;
85+
llvm::StringMap<RefSlabAndCountReferences> FileToRefs;
7886
};
7987

8088
/// This manages symbols from files and an in-memory index on all symbols.

‎clang-tools-extra/clangd/index/IndexAction.cpp

-1
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,6 @@ std::unique_ptr<FrontendAction> createStaticIndexingAction(
186186
IndexOpts.SystemSymbolFilter =
187187
index::IndexingOptions::SystemSymbolFilterKind::All;
188188
Opts.CollectIncludePath = true;
189-
Opts.CountReferences = true;
190189
if (Opts.Origin == SymbolOrigin::Unknown)
191190
Opts.Origin = SymbolOrigin::Static;
192191
Opts.StoreAllDocumentation = false;

‎clang-tools-extra/clangd/indexer/IndexerMain.cpp

+1
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ class IndexActionFactory : public tooling::FrontendActionFactory {
4141

4242
clang::FrontendAction *create() override {
4343
SymbolCollector::Options Opts;
44+
Opts.CountReferences = true;
4445
return createStaticIndexingAction(
4546
Opts,
4647
[&](SymbolSlab S) {

‎clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp

+19-7
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ MATCHER(EmptyIncludeNode, "") {
3232
return !arg.IsTU && !arg.URI.empty() && arg.Digest == FileDigest{{0}} &&
3333
arg.DirectIncludes.empty();
3434
}
35+
MATCHER_P(NumReferences, N, "") { return arg.References == N; }
3536

3637
class MemoryShardStorage : public BackgroundIndexStorage {
3738
mutable std::mutex StorageMu;
@@ -112,6 +113,9 @@ TEST_F(BackgroundIndexTest, IndexTwoFiles) {
112113
#include "A.h"
113114
void f_b() {
114115
(void)common;
116+
(void)common;
117+
(void)common;
118+
(void)common;
115119
})cpp";
116120
llvm::StringMap<std::string> Storage;
117121
size_t CacheHits = 0;
@@ -127,27 +131,35 @@ TEST_F(BackgroundIndexTest, IndexTwoFiles) {
127131
CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
128132

129133
ASSERT_TRUE(Idx.blockUntilIdleForTest());
130-
EXPECT_THAT(
131-
runFuzzyFind(Idx, ""),
132-
UnorderedElementsAre(Named("common"), Named("A_CC"), Named("g"),
133-
AllOf(Named("f_b"), Declared(), Not(Defined()))));
134+
EXPECT_THAT(runFuzzyFind(Idx, ""),
135+
UnorderedElementsAre(AllOf(Named("common"), NumReferences(1U)),
136+
AllOf(Named("A_CC"), NumReferences(0U)),
137+
AllOf(Named("g"), NumReferences(0U)),
138+
AllOf(Named("f_b"), Declared(),
139+
Not(Defined()), NumReferences(0U))));
134140

135141
Cmd.Filename = testPath("root/B.cc");
136142
Cmd.CommandLine = {"clang++", Cmd.Filename};
137-
CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
143+
CDB.setCompileCommand(testPath("root/B.cc"), Cmd);
138144

139145
ASSERT_TRUE(Idx.blockUntilIdleForTest());
140146
// B_CC is dropped as we don't collect symbols from A.h in this compilation.
141147
EXPECT_THAT(runFuzzyFind(Idx, ""),
142-
UnorderedElementsAre(Named("common"), Named("A_CC"), Named("g"),
143-
AllOf(Named("f_b"), Declared(), Defined())));
148+
UnorderedElementsAre(AllOf(Named("common"), NumReferences(5U)),
149+
AllOf(Named("A_CC"), NumReferences(0U)),
150+
AllOf(Named("g"), NumReferences(0U)),
151+
AllOf(Named("f_b"), Declared(), Defined(),
152+
NumReferences(1U))));
144153

145154
auto Syms = runFuzzyFind(Idx, "common");
146155
EXPECT_THAT(Syms, UnorderedElementsAre(Named("common")));
147156
auto Common = *Syms.begin();
148157
EXPECT_THAT(getRefs(Idx, Common.ID),
149158
RefsAre({FileURI("unittest:///root/A.h"),
150159
FileURI("unittest:///root/A.cc"),
160+
FileURI("unittest:///root/B.cc"),
161+
FileURI("unittest:///root/B.cc"),
162+
FileURI("unittest:///root/B.cc"),
151163
FileURI("unittest:///root/B.cc")}));
152164
}
153165

‎clang-tools-extra/clangd/unittests/FileIndexTests.cpp

+35-7
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ MATCHER_P(DefURI, U, "") {
4545
return llvm::StringRef(arg.Definition.FileURI) == U;
4646
}
4747
MATCHER_P(QName, N, "") { return (arg.Scope + arg.Name).str() == N; }
48+
MATCHER_P(NumReferences, N, "") { return arg.References == N; }
4849

4950
namespace clang {
5051
namespace clangd {
@@ -81,7 +82,7 @@ TEST(FileSymbolsTest, UpdateAndGet) {
8182
FileSymbols FS;
8283
EXPECT_THAT(runFuzzyFind(*FS.buildIndex(IndexType::Light), ""), IsEmpty());
8384

84-
FS.update("f1", numSlab(1, 3), refSlab(SymbolID("1"), "f1.cc"));
85+
FS.update("f1", numSlab(1, 3), refSlab(SymbolID("1"), "f1.cc"), false);
8586
EXPECT_THAT(runFuzzyFind(*FS.buildIndex(IndexType::Light), ""),
8687
UnorderedElementsAre(QName("1"), QName("2"), QName("3")));
8788
EXPECT_THAT(getRefs(*FS.buildIndex(IndexType::Light), SymbolID("1")),
@@ -90,8 +91,8 @@ TEST(FileSymbolsTest, UpdateAndGet) {
9091

9192
TEST(FileSymbolsTest, Overlap) {
9293
FileSymbols FS;
93-
FS.update("f1", numSlab(1, 3), nullptr);
94-
FS.update("f2", numSlab(3, 5), nullptr);
94+
FS.update("f1", numSlab(1, 3), nullptr, false);
95+
FS.update("f2", numSlab(3, 5), nullptr, false);
9596
for (auto Type : {IndexType::Light, IndexType::Heavy})
9697
EXPECT_THAT(runFuzzyFind(*FS.buildIndex(Type), ""),
9798
UnorderedElementsAre(QName("1"), QName("2"), QName("3"),
@@ -110,8 +111,8 @@ TEST(FileSymbolsTest, MergeOverlap) {
110111
auto X2 = symbol("x");
111112
X2.Definition.FileURI = "file:///x2";
112113

113-
FS.update("f1", OneSymboSlab(X1), nullptr);
114-
FS.update("f2", OneSymboSlab(X2), nullptr);
114+
FS.update("f1", OneSymboSlab(X1), nullptr, false);
115+
FS.update("f2", OneSymboSlab(X2), nullptr, false);
115116
for (auto Type : {IndexType::Light, IndexType::Heavy})
116117
EXPECT_THAT(
117118
runFuzzyFind(*FS.buildIndex(Type, DuplicateHandling::Merge), "x"),
@@ -123,14 +124,14 @@ TEST(FileSymbolsTest, SnapshotAliveAfterRemove) {
123124
FileSymbols FS;
124125

125126
SymbolID ID("1");
126-
FS.update("f1", numSlab(1, 3), refSlab(ID, "f1.cc"));
127+
FS.update("f1", numSlab(1, 3), refSlab(ID, "f1.cc"), false);
127128

128129
auto Symbols = FS.buildIndex(IndexType::Light);
129130
EXPECT_THAT(runFuzzyFind(*Symbols, ""),
130131
UnorderedElementsAre(QName("1"), QName("2"), QName("3")));
131132
EXPECT_THAT(getRefs(*Symbols, ID), RefsAre({FileURI("f1.cc")}));
132133

133-
FS.update("f1", nullptr, nullptr);
134+
FS.update("f1", nullptr, nullptr, false);
134135
auto Empty = FS.buildIndex(IndexType::Light);
135136
EXPECT_THAT(runFuzzyFind(*Empty, ""), IsEmpty());
136137
EXPECT_THAT(getRefs(*Empty, ID), ElementsAre());
@@ -366,6 +367,33 @@ TEST(FileIndexTest, ReferencesInMainFileWithPreamble) {
366367
RefsAre({RefRange(Main.range())}));
367368
}
368369

370+
TEST(FileSymbolsTest, CountReferencesNoRefSlabs) {
371+
FileSymbols FS;
372+
FS.update("f1", numSlab(1, 3), nullptr, true);
373+
FS.update("f2", numSlab(1, 3), nullptr, false);
374+
EXPECT_THAT(
375+
runFuzzyFind(*FS.buildIndex(IndexType::Light, DuplicateHandling::Merge),
376+
""),
377+
UnorderedElementsAre(AllOf(QName("1"), NumReferences(0u)),
378+
AllOf(QName("2"), NumReferences(0u)),
379+
AllOf(QName("3"), NumReferences(0u))));
380+
}
381+
382+
TEST(FileSymbolsTest, CountReferencesWithRefSlabs) {
383+
FileSymbols FS;
384+
FS.update("f1cpp", numSlab(1, 3), refSlab(SymbolID("1"), "f1.cpp"), true);
385+
FS.update("f1h", numSlab(1, 3), refSlab(SymbolID("1"), "f1.h"), false);
386+
FS.update("f2cpp", numSlab(1, 3), refSlab(SymbolID("2"), "f2.cpp"), true);
387+
FS.update("f2h", numSlab(1, 3), refSlab(SymbolID("2"), "f2.h"), false);
388+
FS.update("f3cpp", numSlab(1, 3), refSlab(SymbolID("3"), "f3.cpp"), true);
389+
FS.update("f3h", numSlab(1, 3), refSlab(SymbolID("3"), "f3.h"), false);
390+
EXPECT_THAT(
391+
runFuzzyFind(*FS.buildIndex(IndexType::Light, DuplicateHandling::Merge),
392+
""),
393+
UnorderedElementsAre(AllOf(QName("1"), NumReferences(1u)),
394+
AllOf(QName("2"), NumReferences(1u)),
395+
AllOf(QName("3"), NumReferences(1u))));
396+
}
369397
} // namespace
370398
} // namespace clangd
371399
} // namespace clang

0 commit comments

Comments
 (0)
Please sign in to comment.