[clang-tools-extra] 1abb883 - [clangd] Don't traverse the AST within uninteresting files during indexing
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Tue May 26 01:34:12 PDT 2020
Author: Sam McCall
Date: 2020-05-26T10:27:28+02:00
New Revision: 1abb883a048153c83a4e11070219d23f362e7377
URL: https://github.com/llvm/llvm-project/commit/1abb883a048153c83a4e11070219d23f362e7377
DIFF: https://github.com/llvm/llvm-project/commit/1abb883a048153c83a4e11070219d23f362e7377.diff
LOG: [clangd] Don't traverse the AST within uninteresting files during indexing
Summary:
We already skip function bodies from these files while parsing, and drop symbols
found in them. However, traversing their ASTs still takes a substantial amount
of time.
Non-scientific benchmark on my machine:
background-indexing llvm-project (llvm+clang+clang-tools-extra), wall time
before: 7:46
after: 5:13
change: -33%
Indexer.cpp libclang should be updated too, I'm less familiar with that code,
and it's doing tricky things with the ShouldSkipFunctionBody callback, so it
needs to be done separately.
Reviewers: kadircet
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D80296
Added:
Modified:
clang-tools-extra/clangd/index/IndexAction.cpp
clang-tools-extra/clangd/unittests/IndexActionTests.cpp
clang/include/clang/Index/IndexingAction.h
clang/include/clang/Index/IndexingOptions.h
clang/lib/Index/IndexDecl.cpp
clang/lib/Index/IndexingAction.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/index/IndexAction.cpp b/clang-tools-extra/clangd/index/IndexAction.cpp
index 9f294d4ab925..aa65008b51c0 100644
--- a/clang-tools-extra/clangd/index/IndexAction.cpp
+++ b/clang-tools-extra/clangd/index/IndexAction.cpp
@@ -132,11 +132,19 @@ class IndexAction : public ASTFrontendAction {
std::function<void(RefSlab)> RefsCallback,
std::function<void(RelationSlab)> RelationsCallback,
std::function<void(IncludeGraph)> IncludeGraphCallback)
- : SymbolsCallback(SymbolsCallback),
- RefsCallback(RefsCallback), RelationsCallback(RelationsCallback),
+ : SymbolsCallback(SymbolsCallback), RefsCallback(RefsCallback),
+ RelationsCallback(RelationsCallback),
IncludeGraphCallback(IncludeGraphCallback), Collector(C),
Includes(std::move(Includes)), Opts(Opts),
- PragmaHandler(collectIWYUHeaderMaps(this->Includes.get())) {}
+ PragmaHandler(collectIWYUHeaderMaps(this->Includes.get())) {
+ this->Opts.ShouldTraverseDecl = [this](const Decl *D) {
+ auto &SM = D->getASTContext().getSourceManager();
+ auto FID = SM.getFileID(SM.getExpansionLoc(D->getLocation()));
+ if (!FID.isValid())
+ return true;
+ return Collector->shouldIndexFile(FID);
+ };
+ }
std::unique_ptr<ASTConsumer>
CreateASTConsumer(CompilerInstance &CI, llvm::StringRef InFile) override {
@@ -146,15 +154,8 @@ class IndexAction : public ASTFrontendAction {
CI.getPreprocessor().addPPCallbacks(
std::make_unique<IncludeGraphCollector>(CI.getSourceManager(), IG));
- return index::createIndexingASTConsumer(
- Collector, Opts, CI.getPreprocessorPtr(),
- /*ShouldSkipFunctionBody=*/[this](const Decl *D) {
- auto &SM = D->getASTContext().getSourceManager();
- auto FID = SM.getFileID(SM.getExpansionLoc(D->getLocation()));
- if (!FID.isValid())
- return false;
- return !Collector->shouldIndexFile(FID);
- });
+ return index::createIndexingASTConsumer(Collector, Opts,
+ CI.getPreprocessorPtr());
}
bool BeginInvocation(CompilerInstance &CI) override {
diff --git a/clang-tools-extra/clangd/unittests/IndexActionTests.cpp b/clang-tools-extra/clangd/unittests/IndexActionTests.cpp
index 6441d019c7e1..31e1bc573290 100644
--- a/clang-tools-extra/clangd/unittests/IndexActionTests.cpp
+++ b/clang-tools-extra/clangd/unittests/IndexActionTests.cpp
@@ -19,6 +19,7 @@ namespace {
using ::testing::AllOf;
using ::testing::ElementsAre;
+using ::testing::EndsWith;
using ::testing::Not;
using ::testing::Pair;
using ::testing::UnorderedElementsAre;
@@ -75,8 +76,7 @@ class IndexActionTest : public ::testing::Test {
new FileManager(FileSystemOptions(), InMemoryFileSystem));
auto Action = createStaticIndexingAction(
- SymbolCollector::Options(),
- [&](SymbolSlab S) { IndexFile.Symbols = std::move(S); },
+ Opts, [&](SymbolSlab S) { IndexFile.Symbols = std::move(S); },
[&](RefSlab R) { IndexFile.Refs = std::move(R); },
[&](RelationSlab R) { IndexFile.Relations = std::move(R); },
[&](IncludeGraph IG) { IndexFile.Sources = std::move(IG); });
@@ -99,11 +99,12 @@ class IndexActionTest : public ::testing::Test {
void addFile(llvm::StringRef Path, llvm::StringRef Content) {
InMemoryFileSystem->addFile(Path, 0,
- llvm::MemoryBuffer::getMemBuffer(Content));
+ llvm::MemoryBuffer::getMemBufferCopy(Content));
FilePaths.push_back(std::string(Path));
}
protected:
+ SymbolCollector::Options Opts;
std::vector<std::string> FilePaths;
llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> InMemoryFileSystem;
};
@@ -250,6 +251,36 @@ TEST_F(IndexActionTest, NoWarnings) {
EXPECT_THAT(*IndexFile.Symbols, ElementsAre(HasName("foo"), HasName("bar")));
}
+TEST_F(IndexActionTest, SkipFiles) {
+ std::string MainFilePath = testPath("main.cpp");
+ addFile(MainFilePath, R"cpp(
+ // clang-format off
+ #include "good.h"
+ #include "bad.h"
+ // clang-format on
+ )cpp");
+ addFile(testPath("good.h"), R"cpp(
+ struct S { int s; };
+ void f1() { S f; }
+ auto unskippable1() { return S(); }
+ )cpp");
+ addFile(testPath("bad.h"), R"cpp(
+ struct T { S t; };
+ void f2() { S f; }
+ auto unskippable2() { return S(); }
+ )cpp");
+ Opts.FileFilter = [](const SourceManager &SM, FileID F) {
+ return !SM.getFileEntryForID(F)->getName().endswith("bad.h");
+ };
+ IndexFileIn IndexFile = runIndexingAction(MainFilePath, {"-std=c++14"});
+ EXPECT_THAT(*IndexFile.Symbols,
+ UnorderedElementsAre(HasName("S"), HasName("s"), HasName("f1"),
+ HasName("unskippable1")));
+ for (const auto &Pair : *IndexFile.Refs)
+ for (const auto &Ref : Pair.second)
+ EXPECT_THAT(Ref.Location.FileURI, EndsWith("good.h"));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
diff --git a/clang/include/clang/Index/IndexingAction.h b/clang/include/clang/Index/IndexingAction.h
index 9ed2a018f161..4baa2d5e7260 100644
--- a/clang/include/clang/Index/IndexingAction.h
+++ b/clang/include/clang/Index/IndexingAction.h
@@ -30,22 +30,21 @@ namespace serialization {
}
namespace index {
- class IndexDataConsumer;
+class IndexDataConsumer;
/// Creates an ASTConsumer that indexes all symbols (macros and AST decls).
+std::unique_ptr<ASTConsumer>
+createIndexingASTConsumer(std::shared_ptr<IndexDataConsumer> DataConsumer,
+ const IndexingOptions &Opts,
+ std::shared_ptr<Preprocessor> PP);
+
std::unique_ptr<ASTConsumer> createIndexingASTConsumer(
std::shared_ptr<IndexDataConsumer> DataConsumer,
const IndexingOptions &Opts, std::shared_ptr<Preprocessor> PP,
+ // Prefer to set Opts.ShouldTraverseDecl and use the above overload.
+ // This version is only needed if used to *track* function body parsing.
std::function<bool(const Decl *)> ShouldSkipFunctionBody);
-inline std::unique_ptr<ASTConsumer> createIndexingASTConsumer(
- std::shared_ptr<IndexDataConsumer> DataConsumer,
- const IndexingOptions &Opts, std::shared_ptr<Preprocessor> PP) {
- return createIndexingASTConsumer(
- std::move(DataConsumer), Opts, std::move(PP),
- /*ShouldSkipFunctionBody=*/[](const Decl *) { return false; });
-}
-
/// Creates a frontend action that indexes all symbols (macros and AST decls).
std::unique_ptr<FrontendAction>
createIndexingAction(std::shared_ptr<IndexDataConsumer> DataConsumer,
diff --git a/clang/include/clang/Index/IndexingOptions.h b/clang/include/clang/Index/IndexingOptions.h
index bbfd6e4a72c6..2dd276998abf 100644
--- a/clang/include/clang/Index/IndexingOptions.h
+++ b/clang/include/clang/Index/IndexingOptions.h
@@ -34,6 +34,12 @@ struct IndexingOptions {
// Has no effect if IndexFunctionLocals are false.
bool IndexParametersInDeclarations = false;
bool IndexTemplateParameters = false;
+
+ // If set, skip indexing inside some declarations for performance.
+ // This prevents traversal, so skipping a struct means its declaration an
+ // members won't be indexed, but references elsewhere to that struct will be.
+ // Currently this is only checked for top-level declarations.
+ std::function<bool(const Decl *)> ShouldTraverseDecl;
};
} // namespace index
diff --git a/clang/lib/Index/IndexDecl.cpp b/clang/lib/Index/IndexDecl.cpp
index 68160bc59eb6..2ba323e63575 100644
--- a/clang/lib/Index/IndexDecl.cpp
+++ b/clang/lib/Index/IndexDecl.cpp
@@ -765,6 +765,9 @@ bool IndexingContext::indexTopLevelDecl(const Decl *D) {
if (isa<ObjCMethodDecl>(D))
return true; // Wait for the objc container.
+ if (IndexOpts.ShouldTraverseDecl && !IndexOpts.ShouldTraverseDecl(D))
+ return true; // skip
+
return indexDecl(D);
}
diff --git a/clang/lib/Index/IndexingAction.cpp b/clang/lib/Index/IndexingAction.cpp
index 4f402135672c..e698c07133a9 100644
--- a/clang/lib/Index/IndexingAction.cpp
+++ b/clang/lib/Index/IndexingAction.cpp
@@ -131,6 +131,21 @@ std::unique_ptr<ASTConsumer> index::createIndexingASTConsumer(
ShouldSkipFunctionBody);
}
+std::unique_ptr<ASTConsumer> clang::index::createIndexingASTConsumer(
+ std::shared_ptr<IndexDataConsumer> DataConsumer,
+ const IndexingOptions &Opts, std::shared_ptr<Preprocessor> PP) {
+ std::function<bool(const Decl *)> ShouldSkipFunctionBody = [](const Decl *) {
+ return false;
+ };
+ if (Opts.ShouldTraverseDecl)
+ ShouldSkipFunctionBody =
+ [ShouldTraverseDecl(Opts.ShouldTraverseDecl)](const Decl *D) {
+ return !ShouldTraverseDecl(D);
+ };
+ return createIndexingASTConsumer(std::move(DataConsumer), Opts, std::move(PP),
+ std::move(ShouldSkipFunctionBody));
+}
+
std::unique_ptr<FrontendAction>
index::createIndexingAction(std::shared_ptr<IndexDataConsumer> DataConsumer,
const IndexingOptions &Opts) {
More information about the cfe-commits
mailing list