sammccall updated this revision to Diff 163730.
sammccall marked 2 inline comments as done.
sammccall added a comment.
Address review comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51221
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/TUScheduler.cpp
clangd/TUScheduler.h
unittests/clangd/FileIndexTests.cpp
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/TUSchedulerTests.cpp
===================================================================
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -45,7 +45,7 @@
TEST_F(TUSchedulerTests, MissingFiles) {
TUScheduler S(getDefaultAsyncThreadsCount(),
- /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+ /*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -101,7 +101,7 @@
Notification Ready;
TUScheduler S(
getDefaultAsyncThreadsCount(),
- /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+ /*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
auto Path = testPath("foo.cpp");
@@ -129,7 +129,7 @@
std::atomic<int> CallbackCount(0);
{
TUScheduler S(getDefaultAsyncThreadsCount(),
- /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+ /*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::seconds(1),
ASTRetentionPolicy());
// FIXME: we could probably use timeouts lower than 1 second here.
@@ -160,7 +160,7 @@
// Run TUScheduler and collect some stats.
{
TUScheduler S(getDefaultAsyncThreadsCount(),
- /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+ /*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::milliseconds(50),
ASTRetentionPolicy());
@@ -258,7 +258,7 @@
Policy.MaxRetainedASTs = 2;
TUScheduler S(
/*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
- noopParsingCallbacks(),
+ /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
llvm::StringLiteral SourceContents = R"cpp(
@@ -308,7 +308,7 @@
TEST_F(TUSchedulerTests, EmptyPreamble) {
TUScheduler S(
/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
- noopParsingCallbacks(),
+ /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -355,7 +355,7 @@
// the same time. All reads should get the same non-null preamble.
TUScheduler S(
/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
- noopParsingCallbacks(),
+ /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
auto Foo = testPath("foo.cpp");
@@ -388,7 +388,7 @@
TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
TUScheduler S(
/*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
- /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
+ /*StorePreambleInMemory=*/true, /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
@@ -441,7 +441,7 @@
TEST_F(TUSchedulerTests, NoChangeDiags) {
TUScheduler S(
/*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
- /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
+ /*StorePreambleInMemory=*/true, /*ASTCallbacks=*/nullptr,
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
Index: unittests/clangd/FileIndexTests.cpp
===================================================================
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -251,11 +251,10 @@
buildPreamble(
FooCpp, *CI, /*OldPreamble=*/nullptr, tooling::CompileCommand(), PI,
std::make_shared<PCHContainerOperations>(), /*StoreInMemory=*/true,
- [&Index, &IndexUpdated](PathRef FilePath, ASTContext &Ctx,
- std::shared_ptr<Preprocessor> PP) {
+ [&](ASTContext &Ctx, std::shared_ptr<Preprocessor> PP) {
EXPECT_FALSE(IndexUpdated) << "Expected only a single index update";
IndexUpdated = true;
- Index.update(FilePath, &Ctx, std::move(PP));
+ Index.update(FooCpp, &Ctx, std::move(PP));
});
ASSERT_TRUE(IndexUpdated);
Index: clangd/TUScheduler.h
===================================================================
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -74,8 +74,6 @@
virtual void onMainAST(PathRef Path, ParsedAST &AST) {}
};
-ParsingCallbacks &noopParsingCallbacks();
-
/// Handles running tasks for ClangdServer and managing the resources (e.g.,
/// preambles and ASTs) for opened files.
/// TUScheduler is not thread-safe, only one thread should be providing updates
@@ -86,7 +84,7 @@
class TUScheduler {
public:
TUScheduler(unsigned AsyncThreadsCount, bool StorePreamblesInMemory,
- ParsingCallbacks &ASTCallbacks,
+ std::unique_ptr<ParsingCallbacks> ASTCallbacks,
std::chrono::steady_clock::duration UpdateDebounce,
ASTRetentionPolicy RetentionPolicy);
~TUScheduler();
@@ -157,7 +155,7 @@
private:
const bool StorePreamblesInMemory;
const std::shared_ptr<PCHContainerOperations> PCHOps;
- ParsingCallbacks &Callbacks;
+ std::unique_ptr<ParsingCallbacks> Callbacks; // not nullptr
Semaphore Barrier;
llvm::StringMap<std::unique_ptr<FileData>> Files;
std::unique_ptr<ASTCache> IdleASTs;
Index: clangd/TUScheduler.cpp
===================================================================
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -361,13 +361,12 @@
std::shared_ptr<const PreambleData> OldPreamble =
getPossiblyStalePreamble();
- std::shared_ptr<const PreambleData> NewPreamble =
- buildPreamble(FileName, *Invocation, OldPreamble, OldCommand, Inputs,
- PCHs, StorePreambleInMemory,
- [this](PathRef Path, ASTContext &Ctx,
- std::shared_ptr<clang::Preprocessor> PP) {
- Callbacks.onPreambleAST(FileName, Ctx, std::move(PP));
- });
+ std::shared_ptr<const PreambleData> NewPreamble = buildPreamble(
+ FileName, *Invocation, OldPreamble, OldCommand, Inputs, PCHs,
+ StorePreambleInMemory,
+ [this](ASTContext &Ctx, std::shared_ptr<clang::Preprocessor> PP) {
+ Callbacks.onPreambleAST(FileName, Ctx, std::move(PP));
+ });
bool CanReuseAST = InputsAreTheSame && (OldPreamble == NewPreamble);
{
@@ -621,11 +620,6 @@
return HardwareConcurrency;
}
-ParsingCallbacks &noopParsingCallbacks() {
- static ParsingCallbacks *Instance = new ParsingCallbacks;
- return *Instance;
-}
-
struct TUScheduler::FileData {
/// Latest inputs, passed to TUScheduler::update().
std::string Contents;
@@ -635,11 +629,13 @@
TUScheduler::TUScheduler(unsigned AsyncThreadsCount,
bool StorePreamblesInMemory,
- ParsingCallbacks &Callbacks,
+ std::unique_ptr<ParsingCallbacks> Callbacks,
std::chrono::steady_clock::duration UpdateDebounce,
ASTRetentionPolicy RetentionPolicy)
: StorePreamblesInMemory(StorePreamblesInMemory),
- PCHOps(std::make_shared<PCHContainerOperations>()), Callbacks(Callbacks),
+ PCHOps(std::make_shared<PCHContainerOperations>()),
+ Callbacks(Callbacks ? move(Callbacks)
+ : llvm::make_unique<ParsingCallbacks>()),
Barrier(AsyncThreadsCount),
IdleASTs(llvm::make_unique<ASTCache>(RetentionPolicy.MaxRetainedASTs)),
UpdateDebounce(UpdateDebounce) {
@@ -678,7 +674,7 @@
// Create a new worker to process the AST-related tasks.
ASTWorkerHandle Worker = ASTWorker::create(
File, *IdleASTs, WorkerThreads ? WorkerThreads.getPointer() : nullptr,
- Barrier, UpdateDebounce, PCHOps, StorePreamblesInMemory, Callbacks);
+ Barrier, UpdateDebounce, PCHOps, StorePreamblesInMemory, *Callbacks);
FD = std::unique_ptr<FileData>(new FileData{
Inputs.Contents, Inputs.CompileCommand, std::move(Worker)});
} else {
Index: clangd/ClangdUnit.h
===================================================================
--- clangd/ClangdUnit.h
+++ clangd/ClangdUnit.h
@@ -127,8 +127,8 @@
IncludeStructure Includes;
};
-using PreambleParsedCallback = std::function<void(
- PathRef Path, ASTContext &, std::shared_ptr<clang::Preprocessor>)>;
+using PreambleParsedCallback =
+ std::function<void(ASTContext &, std::shared_ptr<clang::Preprocessor>)>;
/// Builds compiler invocation that could be used to build AST or preamble.
std::unique_ptr<CompilerInvocation>
Index: clangd/ClangdUnit.cpp
===================================================================
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -95,7 +95,7 @@
if (!ParsedCallback)
return;
trace::Span Tracer("Running PreambleCallback");
- ParsedCallback(File, CI.getASTContext(), CI.getPreprocessorPtr());
+ ParsedCallback(CI.getASTContext(), CI.getPreprocessorPtr());
}
void BeforeExecute(CompilerInstance &CI) override {
Index: clangd/ClangdServer.h
===================================================================
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -190,6 +190,10 @@
/// FIXME: those metrics might be useful too, we should add them.
std::vector<std::pair<Path, std::size_t>> getUsedBytesPerFile() const;
+ /// Returns the active dynamic index if one was built.
+ /// This can be useful for testing, debugging, or observing memory usage.
+ const SymbolIndex *dynamicIndex() const;
+
// Blocks the main thread until the server is idle. Only for use in tests.
// Returns false if the timeout expires.
LLVM_NODISCARD bool
@@ -223,11 +227,10 @@
// - the dynamic index owned by ClangdServer (DynamicIdx)
// - the static index passed to the constructor
// - a merged view of a static and dynamic index (MergedIndex)
- SymbolIndex *Index;
- /// If present, an up-to-date of symbols in open files. Read via Index.
+ const SymbolIndex *Index;
+ // If present, an index of symbols in open files. Read via *Index.
std::unique_ptr<DynamicIndex> DynamicIdx;
- // If present, a merged view of DynamicIdx and an external index. Read via
- // Index.
+ // If present, storage for the merged static/dynamic index. Read via *Index.
std::unique_ptr<SymbolIndex> MergedIndex;
// GUARDED_BY(CachedCompletionFuzzyFindRequestMutex)
Index: clangd/ClangdServer.cpp
===================================================================
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -70,34 +70,57 @@
};
} // namespace
-/// Manages dynamic index for open files. Each file might contribute two sets
-/// of symbols to the dynamic index: symbols from the preamble and symbols
-/// from the file itself. Those have different lifetimes and we merge results
-/// from both
-class ClangdServer::DynamicIndex : public ParsingCallbacks {
+/// The dynamic index tracks symbols visible in open files.
+/// For boring reasons, it doesn't implement SymbolIndex directly - use index().
+class ClangdServer::DynamicIndex {
public:
DynamicIndex(std::vector<std::string> URISchemes)
: PreambleIdx(URISchemes), MainFileIdx(URISchemes),
MergedIndex(mergeIndex(&MainFileIdx, &PreambleIdx)) {}
- SymbolIndex &index() const { return *MergedIndex; }
+ const SymbolIndex &index() const { return *MergedIndex; }
- void onPreambleAST(PathRef Path, ASTContext &Ctx,
- std::shared_ptr<clang::Preprocessor> PP) override {
- PreambleIdx.update(Path, &Ctx, PP, /*TopLevelDecls=*/llvm::None);
- }
+ // Returns callbacks that can be used to update the index with new ASTs.
+ // Index() presents a merged view of the supplied main-file and preamble ASTs.
+ std::unique_ptr<ParsingCallbacks> makeUpdateCallbacks() {
+ struct CB : public ParsingCallbacks {
+ CB(ClangdServer::DynamicIndex *This) : This(This) {}
+ DynamicIndex *This;
- void onMainAST(PathRef Path, ParsedAST &AST) override {
+ void onPreambleAST(PathRef Path, ASTContext &Ctx,
+ std::shared_ptr<clang::Preprocessor> PP) override {
+ This->PreambleIdx.update(Path, &Ctx, std::move(PP));
+ }
- MainFileIdx.update(Path, &AST.getASTContext(), AST.getPreprocessorPtr(),
- AST.getLocalTopLevelDecls());
- }
+ void onMainAST(PathRef Path, ParsedAST &AST) override {
+ This->MainFileIdx.update(Path, &AST.getASTContext(),
+ AST.getPreprocessorPtr(),
+ AST.getLocalTopLevelDecls());
+ }
+ };
+ return llvm::make_unique<CB>(this);
+ };
private:
+ // Contains information from each file's preamble only.
+ // These are large, but update fairly infrequently (preambles are stable).
+ // Missing information:
+ // - symbol occurrences (these are always "from the main file")
+ // - definition locations in the main file
+ //
+ // FIXME: Because the preambles for different TUs have large overlap and
+ // FileIndex doesn't deduplicate, this uses lots of extra RAM.
+ // The biggest obstacle in fixing this: the obvious approach of partitioning
+ // by declaring file (rather than main file) fails if headers provide
+ // different symbols based on preprocessor state.
FileIndex PreambleIdx;
+ // Contains information from each file's main AST.
+ // These are updated frequently (on file change), but are relatively small.
+ // Mostly contains:
+ // - occurrences of symbols declared in the preamble and referenced from main
+ // - symbols declared both in the main file and the preamble
+ // (Note that symbols *only* in the main file are not indexed).
FileIndex MainFileIdx;
- /// Merged view into both indexes. Merges are performed in a similar manner
- /// to the merges of dynamic and static index.
std::unique_ptr<SymbolIndex> MergedIndex;
};
@@ -126,7 +149,7 @@
// FIXME(ioeric): this can be slow and we may be able to index on less
// critical paths.
WorkScheduler(Opts.AsyncThreadsCount, Opts.StorePreamblesInMemory,
- DynamicIdx ? *DynamicIdx : noopParsingCallbacks(),
+ DynamicIdx ? DynamicIdx->makeUpdateCallbacks() : nullptr,
Opts.UpdateDebounce, Opts.RetentionPolicy) {
if (DynamicIdx && Opts.StaticIndex) {
MergedIndex = mergeIndex(&DynamicIdx->index(), Opts.StaticIndex);
@@ -143,6 +166,10 @@
// ClangdServer::DynamicIndex.
ClangdServer::~ClangdServer() = default;
+const SymbolIndex *ClangdServer::dynamicIndex() const {
+ return DynamicIdx ? &DynamicIdx->index() : nullptr;
+}
+
void ClangdServer::setRootPath(PathRef RootPath) {
auto FS = FSProvider.getFileSystem();
auto Status = FS->status(RootPath);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
