From 046557bc03cb9630042c8dcd4d45a4815c383e1e Mon Sep 17 00:00:00 2001 From: Sam McCall Date: Mon, 3 Sep 2018 16:37:59 +0000 Subject: [PATCH] [clangd] Some nitpicking around the new split (preamble/main) dynamic index Summary: - DynamicIndex doesn't implement ParsingCallbacks, to make its role clearer. ParsingCallbacks is a separate object owned by the receiving TUScheduler. (I tried to get rid of the "index-like-object that doesn't implement index" but it was too messy). - Clarified(?) docs around DynamicIndex - fewer details up front, more details inside. - Exposed dynamic index from ClangdServer for memory monitoring and more direct testing of its contents (actual tests not added here, wanted to get this out for review) - Removed a redundant and sligthly confusing filename param in a callback Reviewers: ilya-biryukov Subscribers: javed.absar, ioeric, MaskRay, jkorous, arphaman, kadircet, cfe-commits Differential Revision: https://reviews.llvm.org/D51221 llvm-svn: 341325 --- clang-tools-extra/clangd/ClangdServer.cpp | 61 ++++++++++++++++------ clang-tools-extra/clangd/ClangdServer.h | 11 ++-- clang-tools-extra/clangd/ClangdUnit.cpp | 2 +- clang-tools-extra/clangd/ClangdUnit.h | 4 +- clang-tools-extra/clangd/CodeComplete.cpp | 4 +- clang-tools-extra/clangd/CodeComplete.h | 12 +++-- clang-tools-extra/clangd/TUScheduler.cpp | 26 ++++----- clang-tools-extra/clangd/TUScheduler.h | 6 +-- .../unittests/clangd/FileIndexTests.cpp | 5 +- .../unittests/clangd/TUSchedulerTests.cpp | 20 +++---- 10 files changed, 88 insertions(+), 63 deletions(-) diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp index 7f0b29d..30bef3c 100644 --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -71,34 +71,57 @@ public: }; } // 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 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 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 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 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(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 MergedIndex; }; @@ -127,7 +150,7 @@ ClangdServer::ClangdServer(GlobalCompilationDatabase &CDB, // 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); @@ -144,6 +167,10 @@ ClangdServer::ClangdServer(GlobalCompilationDatabase &CDB, // 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); diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h index 018bddd..50a1aea 100644 --- a/clang-tools-extra/clangd/ClangdServer.h +++ b/clang-tools-extra/clangd/ClangdServer.h @@ -191,6 +191,10 @@ public: /// FIXME: those metrics might be useful too, we should add them. std::vector> 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 @@ -224,11 +228,10 @@ private: // - 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 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 MergedIndex; // GUARDED_BY(CachedCompletionFuzzyFindRequestMutex) diff --git a/clang-tools-extra/clangd/ClangdUnit.cpp b/clang-tools-extra/clangd/ClangdUnit.cpp index ecb597c..6c184ff 100644 --- a/clang-tools-extra/clangd/ClangdUnit.cpp +++ b/clang-tools-extra/clangd/ClangdUnit.cpp @@ -95,7 +95,7 @@ public: if (!ParsedCallback) return; trace::Span Tracer("Running PreambleCallback"); - ParsedCallback(File, CI.getASTContext(), CI.getPreprocessorPtr()); + ParsedCallback(CI.getASTContext(), CI.getPreprocessorPtr()); } void BeforeExecute(CompilerInstance &CI) override { diff --git a/clang-tools-extra/clangd/ClangdUnit.h b/clang-tools-extra/clangd/ClangdUnit.h index 0b12fb1c0..f739270 100644 --- a/clang-tools-extra/clangd/ClangdUnit.h +++ b/clang-tools-extra/clangd/ClangdUnit.h @@ -127,8 +127,8 @@ private: IncludeStructure Includes; }; -using PreambleParsedCallback = std::function)>; +using PreambleParsedCallback = + std::function)>; /// Builds compiler invocation that could be used to build AST or preamble. std::unique_ptr diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp index eda6dfde..8a10899 100644 --- a/clang-tools-extra/clangd/CodeComplete.cpp +++ b/clang-tools-extra/clangd/CodeComplete.cpp @@ -798,7 +798,7 @@ struct ScoredSignature { class SignatureHelpCollector final : public CodeCompleteConsumer { public: SignatureHelpCollector(const clang::CodeCompleteOptions &CodeCompleteOpts, - SymbolIndex *Index, SignatureHelp &SigHelp) + const SymbolIndex *Index, SignatureHelp &SigHelp) : CodeCompleteConsumer(CodeCompleteOpts, /*OutputIsBinary=*/false), SigHelp(SigHelp), @@ -1584,7 +1584,7 @@ SignatureHelp signatureHelp(PathRef FileName, StringRef Contents, Position Pos, IntrusiveRefCntPtr VFS, std::shared_ptr PCHs, - SymbolIndex *Index) { + const SymbolIndex *Index) { SignatureHelp Result; clang::CodeCompleteOptions Options; Options.IncludeGlobals = false; diff --git a/clang-tools-extra/clangd/CodeComplete.h b/clang-tools-extra/clangd/CodeComplete.h index 3213acc..3197e17 100644 --- a/clang-tools-extra/clangd/CodeComplete.h +++ b/clang-tools-extra/clangd/CodeComplete.h @@ -220,11 +220,13 @@ CodeCompleteResult codeComplete(PathRef FileName, SpeculativeFuzzyFind *SpecFuzzyFind = nullptr); /// Get signature help at a specified \p Pos in \p FileName. -SignatureHelp -signatureHelp(PathRef FileName, const tooling::CompileCommand &Command, - PrecompiledPreamble const *Preamble, StringRef Contents, - Position Pos, IntrusiveRefCntPtr VFS, - std::shared_ptr PCHs, SymbolIndex *Index); +SignatureHelp signatureHelp(PathRef FileName, + const tooling::CompileCommand &Command, + PrecompiledPreamble const *Preamble, + StringRef Contents, Position Pos, + IntrusiveRefCntPtr VFS, + std::shared_ptr PCHs, + const SymbolIndex *Index); // For index-based completion, we only consider: // * symbols in namespaces or translation unit scopes (e.g. no class diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp index 48dc445..0ae2a3c 100644 --- a/clang-tools-extra/clangd/TUScheduler.cpp +++ b/clang-tools-extra/clangd/TUScheduler.cpp @@ -365,13 +365,12 @@ void ASTWorker::update( std::shared_ptr OldPreamble = getPossiblyStalePreamble(); - std::shared_ptr NewPreamble = - buildPreamble(FileName, *Invocation, OldPreamble, OldCommand, Inputs, - PCHs, StorePreambleInMemory, - [this](PathRef Path, ASTContext &Ctx, - std::shared_ptr PP) { - Callbacks.onPreambleAST(FileName, Ctx, std::move(PP)); - }); + std::shared_ptr NewPreamble = buildPreamble( + FileName, *Invocation, OldPreamble, OldCommand, Inputs, PCHs, + StorePreambleInMemory, + [this](ASTContext &Ctx, std::shared_ptr PP) { + Callbacks.onPreambleAST(FileName, Ctx, std::move(PP)); + }); bool CanReuseAST = InputsAreTheSame && (OldPreamble == NewPreamble); { @@ -654,11 +653,6 @@ unsigned getDefaultAsyncThreadsCount() { return HardwareConcurrency; } -ParsingCallbacks &noopParsingCallbacks() { - static ParsingCallbacks *Instance = new ParsingCallbacks; - return *Instance; -} - struct TUScheduler::FileData { /// Latest inputs, passed to TUScheduler::update(). std::string Contents; @@ -668,11 +662,13 @@ struct TUScheduler::FileData { TUScheduler::TUScheduler(unsigned AsyncThreadsCount, bool StorePreamblesInMemory, - ParsingCallbacks &Callbacks, + std::unique_ptr Callbacks, std::chrono::steady_clock::duration UpdateDebounce, ASTRetentionPolicy RetentionPolicy) : StorePreamblesInMemory(StorePreamblesInMemory), - PCHOps(std::make_shared()), Callbacks(Callbacks), + PCHOps(std::make_shared()), + Callbacks(Callbacks ? move(Callbacks) + : llvm::make_unique()), Barrier(AsyncThreadsCount), IdleASTs(llvm::make_unique(RetentionPolicy.MaxRetainedASTs)), UpdateDebounce(UpdateDebounce) { @@ -711,7 +707,7 @@ void TUScheduler::update( // 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(new FileData{ Inputs.Contents, Inputs.CompileCommand, std::move(Worker)}); } else { diff --git a/clang-tools-extra/clangd/TUScheduler.h b/clang-tools-extra/clangd/TUScheduler.h index 3ef17c6..e049d3d 100644 --- a/clang-tools-extra/clangd/TUScheduler.h +++ b/clang-tools-extra/clangd/TUScheduler.h @@ -74,8 +74,6 @@ public: 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 @@ ParsingCallbacks &noopParsingCallbacks(); class TUScheduler { public: TUScheduler(unsigned AsyncThreadsCount, bool StorePreamblesInMemory, - ParsingCallbacks &ASTCallbacks, + std::unique_ptr ASTCallbacks, std::chrono::steady_clock::duration UpdateDebounce, ASTRetentionPolicy RetentionPolicy); ~TUScheduler(); @@ -166,7 +164,7 @@ public: private: const bool StorePreamblesInMemory; const std::shared_ptr PCHOps; - ParsingCallbacks &Callbacks; + std::unique_ptr Callbacks; // not nullptr Semaphore Barrier; llvm::StringMap> Files; std::unique_ptr IdleASTs; diff --git a/clang-tools-extra/unittests/clangd/FileIndexTests.cpp b/clang-tools-extra/unittests/clangd/FileIndexTests.cpp index 7861f4c..debca6a 100644 --- a/clang-tools-extra/unittests/clangd/FileIndexTests.cpp +++ b/clang-tools-extra/unittests/clangd/FileIndexTests.cpp @@ -296,11 +296,10 @@ TEST(FileIndexTest, RebuildWithPreamble) { buildPreamble( FooCpp, *CI, /*OldPreamble=*/nullptr, tooling::CompileCommand(), PI, std::make_shared(), /*StoreInMemory=*/true, - [&Index, &IndexUpdated](PathRef FilePath, ASTContext &Ctx, - std::shared_ptr PP) { + [&](ASTContext &Ctx, std::shared_ptr 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); diff --git a/clang-tools-extra/unittests/clangd/TUSchedulerTests.cpp b/clang-tools-extra/unittests/clangd/TUSchedulerTests.cpp index 5222d79..1210b1d 100644 --- a/clang-tools-extra/unittests/clangd/TUSchedulerTests.cpp +++ b/clang-tools-extra/unittests/clangd/TUSchedulerTests.cpp @@ -46,7 +46,7 @@ protected: TEST_F(TUSchedulerTests, MissingFiles) { TUScheduler S(getDefaultAsyncThreadsCount(), - /*StorePreamblesInMemory=*/true, noopParsingCallbacks(), + /*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr, /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), ASTRetentionPolicy()); @@ -104,7 +104,7 @@ TEST_F(TUSchedulerTests, WantDiagnostics) { 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"); @@ -132,7 +132,7 @@ TEST_F(TUSchedulerTests, Debounce) { std::atomic 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. @@ -165,7 +165,7 @@ TEST_F(TUSchedulerTests, PreambleConsistency) { Notification InconsistentReadDone; // Must live longest. TUScheduler S( getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true, - noopParsingCallbacks(), + /*ASTCallbacks=*/nullptr, /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), ASTRetentionPolicy()); auto Path = testPath("foo.cpp"); @@ -221,7 +221,7 @@ TEST_F(TUSchedulerTests, ManyUpdates) { // Run TUScheduler and collect some stats. { TUScheduler S(getDefaultAsyncThreadsCount(), - /*StorePreamblesInMemory=*/true, noopParsingCallbacks(), + /*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr, /*UpdateDebounce=*/std::chrono::milliseconds(50), ASTRetentionPolicy()); @@ -319,7 +319,7 @@ TEST_F(TUSchedulerTests, EvictedAST) { 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( @@ -369,7 +369,7 @@ TEST_F(TUSchedulerTests, EvictedAST) { TEST_F(TUSchedulerTests, EmptyPreamble) { TUScheduler S( /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true, - noopParsingCallbacks(), + /*ASTCallbacks=*/nullptr, /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), ASTRetentionPolicy()); @@ -416,7 +416,7 @@ TEST_F(TUSchedulerTests, RunWaitsForPreamble) { // 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"); @@ -449,7 +449,7 @@ TEST_F(TUSchedulerTests, RunWaitsForPreamble) { TEST_F(TUSchedulerTests, NoopOnEmptyChanges) { TUScheduler S( /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(), - /*StorePreambleInMemory=*/true, noopParsingCallbacks(), + /*StorePreambleInMemory=*/true, /*ASTCallbacks=*/nullptr, /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), ASTRetentionPolicy()); @@ -502,7 +502,7 @@ TEST_F(TUSchedulerTests, NoopOnEmptyChanges) { TEST_F(TUSchedulerTests, NoChangeDiags) { TUScheduler S( /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(), - /*StorePreambleInMemory=*/true, noopParsingCallbacks(), + /*StorePreambleInMemory=*/true, /*ASTCallbacks=*/nullptr, /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), ASTRetentionPolicy()); -- 2.7.4