Skip to content

Commit

Permalink
[clangd] Count number of references while merging RefSlabs inside Fil…
Browse files Browse the repository at this point in the history 
…eIndex

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
  • Loading branch information
@kadircet @MrSidims
kadircet authored and MrSidims committed May 24, 2019
1 parent b02b49c commit 9d88aaa
Show file tree
Hide file tree
Showing 7 changed files with 104 additions and 33 deletions.
10 changes: 7 additions & 3 deletions clang-tools-extra/clangd/index/Background.cpp
View file
Expand Up @@ -356,7 +356,8 @@ void BackgroundIndex::update(llvm::StringRef MainFile, IndexFileIn Index,
// This can override a newer version that is added in another thread, if
// this thread sees the older version but finishes later. This should be
// rare in practice.
IndexedSymbols.update(Path, std::move(SS), std::move(RS));
IndexedSymbols.update(Path, std::move(SS), std::move(RS),
Path == MainFile);
}
}
}
Expand Down Expand Up @@ -478,7 +479,8 @@ BackgroundIndex::loadShard(const tooling::CompileCommand &Cmd,
struct ShardInfo {
std::string AbsolutePath;
std::unique_ptr<IndexFileIn> Shard;
FileDigest Digest;
FileDigest Digest = {};
bool CountReferences = false;
};
std::vector<ShardInfo> IntermediateSymbols;
// Make sure we don't have duplicate elements in the queue. Keys are absolute
Expand Down Expand Up @@ -539,6 +541,7 @@ BackgroundIndex::loadShard(const tooling::CompileCommand &Cmd,
SI.AbsolutePath = CurDependency.Path;
SI.Shard = std::move(Shard);
SI.Digest = I.getValue().Digest;
SI.CountReferences = I.getValue().IsTU;
IntermediateSymbols.push_back(std::move(SI));
// Check if the source needs re-indexing.
// Get the digest, skip it if file doesn't exist.
Expand Down Expand Up @@ -568,7 +571,8 @@ BackgroundIndex::loadShard(const tooling::CompileCommand &Cmd,
? llvm::make_unique<RefSlab>(std::move(*SI.Shard->Refs))
: nullptr;
IndexedFileDigests[SI.AbsolutePath] = SI.Digest;
IndexedSymbols.update(SI.AbsolutePath, std::move(SS), std::move(RS));
IndexedSymbols.update(SI.AbsolutePath, std::move(SS), std::move(RS),
SI.CountReferences);
}
}

Expand Down
43 changes: 31 additions & 12 deletions clang-tools-extra/clangd/index/FileIndex.cpp
View file
Expand Up @@ -90,28 +90,36 @@ SymbolSlab indexHeaderSymbols(ASTContext &AST, std::shared_ptr<Preprocessor> PP,
}

void FileSymbols::update(PathRef Path, std::unique_ptr<SymbolSlab> Symbols,
std::unique_ptr<RefSlab> Refs) {
std::unique_ptr<RefSlab> Refs, bool CountReferences) {
std::lock_guard<std::mutex> Lock(Mutex);
if (!Symbols)
FileToSymbols.erase(Path);
else
FileToSymbols[Path] = std::move(Symbols);
if (!Refs)
if (!Refs) {
FileToRefs.erase(Path);
else
FileToRefs[Path] = std::move(Refs);
return;
}
RefSlabAndCountReferences Item;
Item.CountReferences = CountReferences;
Item.Slab = std::move(Refs);
FileToRefs[Path] = std::move(Item);
}

std::unique_ptr<SymbolIndex>
FileSymbols::buildIndex(IndexType Type, DuplicateHandling DuplicateHandle) {
std::vector<std::shared_ptr<SymbolSlab>> SymbolSlabs;
std::vector<std::shared_ptr<RefSlab>> RefSlabs;
std::vector<RefSlab *> MainFileRefs;
{
std::lock_guard<std::mutex> Lock(Mutex);
for (const auto &FileAndSymbols : FileToSymbols)
SymbolSlabs.push_back(FileAndSymbols.second);
for (const auto &FileAndRefs : FileToRefs)
RefSlabs.push_back(FileAndRefs.second);
for (const auto &FileAndRefs : FileToRefs) {
RefSlabs.push_back(FileAndRefs.second.Slab);
if (FileAndRefs.second.CountReferences)
MainFileRefs.push_back(RefSlabs.back().get());
}
}
std::vector<const Symbol *> AllSymbols;
std::vector<Symbol> SymsStorage;
Expand All @@ -120,25 +128,35 @@ FileSymbols::buildIndex(IndexType Type, DuplicateHandling DuplicateHandle) {
llvm::DenseMap<SymbolID, Symbol> Merged;
for (const auto &Slab : SymbolSlabs) {
for (const auto &Sym : *Slab) {
assert(Sym.References == 0 &&
"Symbol with non-zero references sent to FileSymbols");
auto I = Merged.try_emplace(Sym.ID, Sym);
if (!I.second)
I.first->second = mergeSymbol(I.first->second, Sym);
}
}
for (const RefSlab *Refs : MainFileRefs)
for (const auto &Sym : *Refs) {
auto It = Merged.find(Sym.first);
assert(It != Merged.end() && "Reference to unknown symbol");
It->getSecond().References += Sym.second.size();
}
SymsStorage.reserve(Merged.size());
for (auto &Sym : Merged) {
SymsStorage.push_back(std::move(Sym.second));
AllSymbols.push_back(&SymsStorage.back());
}
// FIXME: aggregate symbol reference count based on references.
break;
}
case DuplicateHandling::PickOne: {
llvm::DenseSet<SymbolID> AddedSymbols;
for (const auto &Slab : SymbolSlabs)
for (const auto &Sym : *Slab)
for (const auto &Sym : *Slab) {
assert(Sym.References == 0 &&
"Symbol with non-zero references sent to FileSymbols");
if (AddedSymbols.insert(Sym.ID).second)
AllSymbols.push_back(&Sym);
}
break;
}
}
Expand Down Expand Up @@ -201,9 +219,9 @@ void FileIndex::updatePreamble(PathRef Path, ASTContext &AST,
std::shared_ptr<Preprocessor> PP,
const CanonicalIncludes &Includes) {
auto Symbols = indexHeaderSymbols(AST, std::move(PP), Includes);
PreambleSymbols.update(Path,
llvm::make_unique<SymbolSlab>(std::move(Symbols)),
llvm::make_unique<RefSlab>());
PreambleSymbols.update(
Path, llvm::make_unique<SymbolSlab>(std::move(Symbols)),
llvm::make_unique<RefSlab>(), /*CountReferences=*/false);
PreambleIndex.reset(
PreambleSymbols.buildIndex(UseDex ? IndexType::Heavy : IndexType::Light,
DuplicateHandling::PickOne));
Expand All @@ -213,7 +231,8 @@ void FileIndex::updateMain(PathRef Path, ParsedAST &AST) {
auto Contents = indexMainDecls(AST);
MainFileSymbols.update(
Path, llvm::make_unique<SymbolSlab>(std::move(Contents.first)),
llvm::make_unique<RefSlab>(std::move(Contents.second)));
llvm::make_unique<RefSlab>(std::move(Contents.second)),
/*CountReferences=*/true);
MainFileIndex.reset(
MainFileSymbols.buildIndex(IndexType::Light, DuplicateHandling::PickOne));
}
Expand Down
14 changes: 11 additions & 3 deletions clang-tools-extra/clangd/index/FileIndex.h
View file
Expand Up @@ -60,21 +60,29 @@ class FileSymbols {
public:
/// Updates all symbols and refs in a file.
/// If either is nullptr, corresponding data for \p Path will be removed.
/// If CountReferences is true, \p Refs will be used for counting References
/// during merging.
void update(PathRef Path, std::unique_ptr<SymbolSlab> Slab,
std::unique_ptr<RefSlab> Refs);
std::unique_ptr<RefSlab> Refs, bool CountReferences);

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

private:
struct RefSlabAndCountReferences {
std::shared_ptr<RefSlab> Slab;
bool CountReferences = false;
};
mutable std::mutex Mutex;

/// Stores the latest symbol snapshots for all active files.
llvm::StringMap<std::shared_ptr<SymbolSlab>> FileToSymbols;
/// Stores the latest ref snapshots for all active files.
llvm::StringMap<std::shared_ptr<RefSlab>> FileToRefs;
llvm::StringMap<RefSlabAndCountReferences> FileToRefs;
};

/// This manages symbols from files and an in-memory index on all symbols.
Expand Down
1 change: 0 additions & 1 deletion clang-tools-extra/clangd/index/IndexAction.cpp
View file
Expand Up @@ -186,7 +186,6 @@ std::unique_ptr<FrontendAction> createStaticIndexingAction(
IndexOpts.SystemSymbolFilter =
index::IndexingOptions::SystemSymbolFilterKind::All;
Opts.CollectIncludePath = true;
Opts.CountReferences = true;
if (Opts.Origin == SymbolOrigin::Unknown)
Opts.Origin = SymbolOrigin::Static;
Opts.StoreAllDocumentation = false;
Expand Down
1 change: 1 addition & 0 deletions clang-tools-extra/clangd/indexer/IndexerMain.cpp
View file
Expand Up @@ -41,6 +41,7 @@ class IndexActionFactory : public tooling::FrontendActionFactory {

clang::FrontendAction *create() override {
SymbolCollector::Options Opts;
Opts.CountReferences = true;
return createStaticIndexingAction(
Opts,
[&](SymbolSlab S) {
Expand Down
26 changes: 19 additions & 7 deletions clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
View file
Expand Up @@ -32,6 +32,7 @@ MATCHER(EmptyIncludeNode, "") {
return !arg.IsTU && !arg.URI.empty() && arg.Digest == FileDigest{{0}} &&
arg.DirectIncludes.empty();
}
MATCHER_P(NumReferences, N, "") { return arg.References == N; }

class MemoryShardStorage : public BackgroundIndexStorage {
mutable std::mutex StorageMu;
Expand Down Expand Up @@ -112,6 +113,9 @@ TEST_F(BackgroundIndexTest, IndexTwoFiles) {
#include "A.h"
void f_b() {
(void)common;
(void)common;
(void)common;
(void)common;
})cpp";
llvm::StringMap<std::string> Storage;
size_t CacheHits = 0;
Expand All @@ -127,27 +131,35 @@ TEST_F(BackgroundIndexTest, IndexTwoFiles) {
CDB.setCompileCommand(testPath("root/A.cc"), Cmd);

ASSERT_TRUE(Idx.blockUntilIdleForTest());
EXPECT_THAT(
runFuzzyFind(Idx, ""),
UnorderedElementsAre(Named("common"), Named("A_CC"), Named("g"),
AllOf(Named("f_b"), Declared(), Not(Defined()))));
EXPECT_THAT(runFuzzyFind(Idx, ""),
UnorderedElementsAre(AllOf(Named("common"), NumReferences(1U)),
AllOf(Named("A_CC"), NumReferences(0U)),
AllOf(Named("g"), NumReferences(0U)),
AllOf(Named("f_b"), Declared(),
Not(Defined()), NumReferences(0U))));

Cmd.Filename = testPath("root/B.cc");
Cmd.CommandLine = {"clang++", Cmd.Filename};
CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
CDB.setCompileCommand(testPath("root/B.cc"), Cmd);

ASSERT_TRUE(Idx.blockUntilIdleForTest());
// B_CC is dropped as we don't collect symbols from A.h in this compilation.
EXPECT_THAT(runFuzzyFind(Idx, ""),
UnorderedElementsAre(Named("common"), Named("A_CC"), Named("g"),
AllOf(Named("f_b"), Declared(), Defined())));
UnorderedElementsAre(AllOf(Named("common"), NumReferences(5U)),
AllOf(Named("A_CC"), NumReferences(0U)),
AllOf(Named("g"), NumReferences(0U)),
AllOf(Named("f_b"), Declared(), Defined(),
NumReferences(1U))));

auto Syms = runFuzzyFind(Idx, "common");
EXPECT_THAT(Syms, UnorderedElementsAre(Named("common")));
auto Common = *Syms.begin();
EXPECT_THAT(getRefs(Idx, Common.ID),
RefsAre({FileURI("unittest:///root/A.h"),
FileURI("unittest:///root/A.cc"),
FileURI("unittest:///root/B.cc"),
FileURI("unittest:///root/B.cc"),
FileURI("unittest:///root/B.cc"),
FileURI("unittest:///root/B.cc")}));
}

Expand Down
42 changes: 35 additions & 7 deletions clang-tools-extra/clangd/unittests/FileIndexTests.cpp
View file
Expand Up @@ -45,6 +45,7 @@ MATCHER_P(DefURI, U, "") {
return llvm::StringRef(arg.Definition.FileURI) == U;
}
MATCHER_P(QName, N, "") { return (arg.Scope + arg.Name).str() == N; }
MATCHER_P(NumReferences, N, "") { return arg.References == N; }

namespace clang {
namespace clangd {
Expand Down Expand Up @@ -81,7 +82,7 @@ TEST(FileSymbolsTest, UpdateAndGet) {
FileSymbols FS;
EXPECT_THAT(runFuzzyFind(*FS.buildIndex(IndexType::Light), ""), IsEmpty());

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

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

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

SymbolID ID("1");
FS.update("f1", numSlab(1, 3), refSlab(ID, "f1.cc"));
FS.update("f1", numSlab(1, 3), refSlab(ID, "f1.cc"), false);

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

FS.update("f1", nullptr, nullptr);
FS.update("f1", nullptr, nullptr, false);
auto Empty = FS.buildIndex(IndexType::Light);
EXPECT_THAT(runFuzzyFind(*Empty, ""), IsEmpty());
EXPECT_THAT(getRefs(*Empty, ID), ElementsAre());
Expand Down Expand Up @@ -366,6 +367,33 @@ TEST(FileIndexTest, ReferencesInMainFileWithPreamble) {
RefsAre({RefRange(Main.range())}));
}

TEST(FileSymbolsTest, CountReferencesNoRefSlabs) {
FileSymbols FS;
FS.update("f1", numSlab(1, 3), nullptr, true);
FS.update("f2", numSlab(1, 3), nullptr, false);
EXPECT_THAT(
runFuzzyFind(*FS.buildIndex(IndexType::Light, DuplicateHandling::Merge),
""),
UnorderedElementsAre(AllOf(QName("1"), NumReferences(0u)),
AllOf(QName("2"), NumReferences(0u)),
AllOf(QName("3"), NumReferences(0u))));
}

TEST(FileSymbolsTest, CountReferencesWithRefSlabs) {
FileSymbols FS;
FS.update("f1cpp", numSlab(1, 3), refSlab(SymbolID("1"), "f1.cpp"), true);
FS.update("f1h", numSlab(1, 3), refSlab(SymbolID("1"), "f1.h"), false);
FS.update("f2cpp", numSlab(1, 3), refSlab(SymbolID("2"), "f2.cpp"), true);
FS.update("f2h", numSlab(1, 3), refSlab(SymbolID("2"), "f2.h"), false);
FS.update("f3cpp", numSlab(1, 3), refSlab(SymbolID("3"), "f3.cpp"), true);
FS.update("f3h", numSlab(1, 3), refSlab(SymbolID("3"), "f3.h"), false);
EXPECT_THAT(
runFuzzyFind(*FS.buildIndex(IndexType::Light, DuplicateHandling::Merge),
""),
UnorderedElementsAre(AllOf(QName("1"), NumReferences(1u)),
AllOf(QName("2"), NumReferences(1u)),
AllOf(QName("3"), NumReferences(1u))));
}
} // namespace
} // namespace clangd
} // namespace clang

0 comments on commit 9d88aaa

Please sign in to comment.