sammccall created this revision.
sammccall added a reviewer: hokein.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay,
ioeric, ilya-biryukov.
Reuse the old -use-dex-index experiment flag for this.
To avoid breaking the tests, make Dex deduplicate symbols, addressing an old
FIXME.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53288
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/index/Background.cpp
clangd/index/FileIndex.cpp
clangd/index/FileIndex.h
clangd/index/dex/Dex.h
clangd/tool/ClangdMain.cpp
unittests/clangd/DexTests.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/TestTU.cpp
Index: unittests/clangd/TestTU.cpp
===================================================================
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -50,7 +50,8 @@
std::unique_ptr<SymbolIndex> TestTU::index() const {
auto AST = build();
- auto Idx = llvm::make_unique<FileIndex>();
+ auto Idx = llvm::make_unique<FileIndex>(
+ /*URISchemes=*/std::vector<std::string>{}, /*UseDex=*/true);
Idx->updatePreamble(Filename, AST.getASTContext(), AST.getPreprocessorPtr());
Idx->updateMain(Filename, AST);
return std::move(Idx);
Index: unittests/clangd/FileIndexTests.cpp
===================================================================
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -86,35 +86,37 @@
TEST(FileSymbolsTest, UpdateAndGet) {
FileSymbols FS;
- EXPECT_THAT(getSymbolNames(*FS.buildMemIndex()), UnorderedElementsAre());
+ EXPECT_THAT(getSymbolNames(*FS.buildIndex(IndexType::Light)),
+ UnorderedElementsAre());
FS.update("f1", numSlab(1, 3), refSlab(SymbolID("1"), "f1.cc"));
- EXPECT_THAT(getSymbolNames(*FS.buildMemIndex()),
+ EXPECT_THAT(getSymbolNames(*FS.buildIndex(IndexType::Light)),
UnorderedElementsAre("1", "2", "3"));
- EXPECT_THAT(getRefs(*FS.buildMemIndex(), SymbolID("1")),
+ EXPECT_THAT(getRefs(*FS.buildIndex(IndexType::Light), SymbolID("1")),
RefsAre({FileURI("f1.cc")}));
}
TEST(FileSymbolsTest, Overlap) {
FileSymbols FS;
FS.update("f1", numSlab(1, 3), nullptr);
FS.update("f2", numSlab(3, 5), nullptr);
- EXPECT_THAT(getSymbolNames(*FS.buildMemIndex()),
- UnorderedElementsAre("1", "2", "3", "4", "5"));
+ for (auto Type : {IndexType::Light, IndexType::Heavy})
+ EXPECT_THAT(getSymbolNames(*FS.buildIndex(Type)),
+ UnorderedElementsAre("1", "2", "3", "4", "5"));
}
TEST(FileSymbolsTest, SnapshotAliveAfterRemove) {
FileSymbols FS;
SymbolID ID("1");
FS.update("f1", numSlab(1, 3), refSlab(ID, "f1.cc"));
- auto Symbols = FS.buildMemIndex();
+ auto Symbols = FS.buildIndex(IndexType::Light);
EXPECT_THAT(getSymbolNames(*Symbols), UnorderedElementsAre("1", "2", "3"));
EXPECT_THAT(getRefs(*Symbols, ID), RefsAre({FileURI("f1.cc")}));
FS.update("f1", nullptr, nullptr);
- auto Empty = FS.buildMemIndex();
+ auto Empty = FS.buildIndex(IndexType::Light);
EXPECT_THAT(getSymbolNames(*Empty), UnorderedElementsAre());
EXPECT_THAT(getRefs(*Empty, ID), ElementsAre());
Index: unittests/clangd/DexTests.cpp
===================================================================
--- unittests/clangd/DexTests.cpp
+++ unittests/clangd/DexTests.cpp
@@ -492,19 +492,13 @@
"other::A"));
}
-// FIXME(kbobyrev): This test is different for Dex and MemIndex: while
-// MemIndex manages response deduplication, Dex simply returns all matched
-// symbols which means there might be equivalent symbols in the response.
-// Before drop-in replacement of MemIndex with Dex happens, FileIndex
-// should handle deduplication instead.
TEST(DexTest, DexDeduplicate) {
std::vector<Symbol> Symbols = {symbol("1"), symbol("2"), symbol("3"),
symbol("2") /* duplicate */};
FuzzyFindRequest Req;
Req.Query = "2";
Dex I(Symbols, RefSlab(), URISchemes);
- EXPECT_FALSE(Req.Limit);
- EXPECT_THAT(match(I, Req), ElementsAre("2", "2"));
+ EXPECT_THAT(match(I, Req), ElementsAre("2"));
}
TEST(DexTest, DexLimitedNumMatches) {
Index: clangd/tool/ClangdMain.cpp
===================================================================
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -29,11 +29,11 @@
using namespace clang;
using namespace clang::clangd;
-// FIXME: remove this option when Dex is stable enough.
+// FIXME: remove this option when Dex is cheap enough.
static llvm::cl::opt<bool>
UseDex("use-dex-index",
- llvm::cl::desc("Use experimental Dex static index."),
- llvm::cl::init(true), llvm::cl::Hidden);
+ llvm::cl::desc("Use experimental Dex dynamic index."),
+ llvm::cl::init(false), llvm::cl::Hidden);
static llvm::cl::opt<Path> CompileCommandsDir(
"compile-commands-dir",
@@ -286,14 +286,15 @@
if (!ResourceDir.empty())
Opts.ResourceDir = ResourceDir;
Opts.BuildDynamicSymbolIndex = EnableIndex;
+ Opts.HeavyweightDynamicSymbolIndex = UseDex;
std::unique_ptr<SymbolIndex> StaticIdx;
std::future<void> AsyncIndexLoad; // Block exit while loading the index.
if (EnableIndex && !IndexFile.empty()) {
// Load the index asynchronously. Meanwhile SwapIndex returns no results.
SwapIndex *Placeholder;
StaticIdx.reset(Placeholder = new SwapIndex(llvm::make_unique<MemIndex>()));
AsyncIndexLoad = runAsync<void>([Placeholder, &Opts] {
- if (auto Idx = loadIndex(IndexFile, Opts.URISchemes, UseDex))
+ if (auto Idx = loadIndex(IndexFile, Opts.URISchemes, /*UseDex=*/true))
Placeholder->reset(std::move(Idx));
});
if (RunSynchronously)
Index: clangd/index/dex/Dex.h
===================================================================
--- clangd/index/dex/Dex.h
+++ clangd/index/dex/Dex.h
@@ -52,8 +52,10 @@
SymbolCollector::Options Opts;
URISchemes = Opts.URISchemes;
}
+ llvm::DenseSet<SymbolID> SeenIDs;
for (auto &&Sym : Symbols)
- this->Symbols.push_back(&Sym);
+ if (SeenIDs.insert(Sym.ID).second)
+ this->Symbols.push_back(&Sym);
for (auto &&Ref : Refs)
this->Refs.try_emplace(Ref.first, Ref.second);
buildIndex();
Index: clangd/index/FileIndex.h
===================================================================
--- clangd/index/FileIndex.h
+++ clangd/index/FileIndex.h
@@ -26,6 +26,14 @@
namespace clang {
namespace clangd {
+/// Select between in-memory index implementations, which have tradeoffs.
+enum class IndexType {
+ // MemIndex is trivially cheap to build, but has poor query performance.
+ Light,
+ // Dex is relatively expensive to build and uses more memory, but is fast.
+ Heavy,
+};
+
/// 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.
@@ -47,7 +55,8 @@
std::unique_ptr<RefSlab> Refs);
// The index keeps the symbols alive.
- std::unique_ptr<SymbolIndex> buildMemIndex();
+ std::unique_ptr<SymbolIndex>
+ buildIndex(IndexType, ArrayRef<std::string> URISchemes = {});
private:
mutable std::mutex Mutex;
@@ -64,7 +73,7 @@
public:
/// If URISchemes is empty, the default schemes in SymbolCollector will be
/// used.
- FileIndex(std::vector<std::string> URISchemes = {});
+ FileIndex(std::vector<std::string> URISchemes = {}, bool UseDex = true);
/// Update preamble symbols of file \p Path with all declarations in \p AST
/// and macros in \p PP.
@@ -76,6 +85,7 @@
void updateMain(PathRef Path, ParsedAST &AST);
private:
+ bool UseDex; // FIXME: this should be always on.
std::vector<std::string> URISchemes;
// Contains information from each file's preamble only.
Index: clangd/index/FileIndex.cpp
===================================================================
--- clangd/index/FileIndex.cpp
+++ clangd/index/FileIndex.cpp
@@ -13,6 +13,7 @@
#include "SymbolCollector.h"
#include "index/Index.h"
#include "index/Merge.h"
+#include "index/dex/Dex.h"
#include "clang/Index/IndexingAction.h"
#include "clang/Lex/MacroInfo.h"
#include "clang/Lex/Preprocessor.h"
@@ -98,7 +99,8 @@
FileToRefs[Path] = std::move(Refs);
}
-std::unique_ptr<SymbolIndex> FileSymbols::buildMemIndex() {
+std::unique_ptr<SymbolIndex>
+FileSymbols::buildIndex(IndexType Type, ArrayRef<std::string> URISchemes) {
std::vector<std::shared_ptr<SymbolSlab>> SymbolSlabs;
std::vector<std::shared_ptr<RefSlab>> RefSlabs;
{
@@ -144,34 +146,44 @@
StorageSize += RefSlab->bytes();
// Index must keep the slabs and contiguous ranges alive.
- return llvm::make_unique<MemIndex>(
- llvm::make_pointee_range(AllSymbols), std::move(AllRefs),
- std::make_tuple(std::move(SymbolSlabs), std::move(RefSlabs),
- std::move(RefsStorage)),
- StorageSize);
+ switch (Type) {
+ case IndexType::Light:
+ return llvm::make_unique<MemIndex>(
+ llvm::make_pointee_range(AllSymbols), std::move(AllRefs),
+ std::make_tuple(std::move(SymbolSlabs), std::move(RefSlabs),
+ std::move(RefsStorage)),
+ StorageSize);
+ case IndexType::Heavy:
+ return llvm::make_unique<dex::Dex>(
+ llvm::make_pointee_range(AllSymbols), std::move(AllRefs),
+ std::make_tuple(std::move(SymbolSlabs), std::move(RefSlabs),
+ std::move(RefsStorage)),
+ StorageSize, std::move(URISchemes));
+ }
}
-FileIndex::FileIndex(std::vector<std::string> URISchemes)
- : MergedIndex(&MainFileIndex, &PreambleIndex),
+FileIndex::FileIndex(std::vector<std::string> URISchemes, bool UseDex)
+ : MergedIndex(&MainFileIndex, &PreambleIndex), UseDex(UseDex),
URISchemes(std::move(URISchemes)),
- PreambleIndex(PreambleSymbols.buildMemIndex()),
- MainFileIndex(MainFileSymbols.buildMemIndex()) {}
+ PreambleIndex(llvm::make_unique<MemIndex>()),
+ MainFileIndex(llvm::make_unique<MemIndex>()) {}
void FileIndex::updatePreamble(PathRef Path, ASTContext &AST,
std::shared_ptr<Preprocessor> PP) {
auto Symbols = indexHeaderSymbols(AST, std::move(PP), URISchemes);
PreambleSymbols.update(Path,
llvm::make_unique<SymbolSlab>(std::move(Symbols)),
llvm::make_unique<RefSlab>());
- PreambleIndex.reset(PreambleSymbols.buildMemIndex());
+ PreambleIndex.reset(PreambleSymbols.buildIndex(
+ UseDex ? IndexType::Heavy : IndexType::Light, URISchemes));
}
void FileIndex::updateMain(PathRef Path, ParsedAST &AST) {
auto Contents = indexMainDecls(AST, URISchemes);
MainFileSymbols.update(
Path, llvm::make_unique<SymbolSlab>(std::move(Contents.first)),
llvm::make_unique<RefSlab>(std::move(Contents.second)));
- MainFileIndex.reset(MainFileSymbols.buildMemIndex());
+ MainFileIndex.reset(MainFileSymbols.buildIndex(IndexType::Light, URISchemes));
}
} // namespace clangd
Index: clangd/index/Background.cpp
===================================================================
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -183,7 +183,7 @@
// FIXME: this should rebuild once-in-a-while, not after every file.
// At that point we should use Dex, too.
vlog("Rebuilding automatic index");
- reset(IndexedSymbols.buildMemIndex());
+ reset(IndexedSymbols.buildIndex(IndexType::Light));
return Error::success();
}
Index: clangd/ClangdServer.h
===================================================================
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -75,6 +75,9 @@
/// If true, ClangdServer builds a dynamic in-memory index for symbols in
/// opened files and uses the index to augment code completion results.
bool BuildDynamicSymbolIndex = false;
+ /// Use a heavier and faster in-memory index implementation.
+ /// FIXME: we should make this true if it isn't too slow!.
+ bool HeavyweightDynamicSymbolIndex = false;
/// URI schemes to use when building the dynamic index.
/// If empty, the default schemes in SymbolCollector will be used.
Index: clangd/ClangdServer.cpp
===================================================================
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -105,8 +105,10 @@
: CDB(CDB), DiagConsumer(DiagConsumer), FSProvider(FSProvider),
ResourceDir(Opts.ResourceDir ? Opts.ResourceDir->str()
: getStandardResourceDir()),
- DynamicIdx(Opts.BuildDynamicSymbolIndex ? new FileIndex(Opts.URISchemes)
- : nullptr),
+ DynamicIdx(Opts.BuildDynamicSymbolIndex
+ ? new FileIndex(Opts.URISchemes,
+ Opts.HeavyweightDynamicSymbolIndex)
+ : nullptr),
PCHs(std::make_shared<PCHContainerOperations>()),
// Pass a callback into `WorkScheduler` to extract symbols from a newly
// parsed file and rebuild the file index synchronously each time an AST
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
