Index: clangd/TUScheduler.cpp =================================================================== --- clangd/TUScheduler.cpp +++ clangd/TUScheduler.cpp @@ -324,6 +324,11 @@ ParseInputs Inputs, WantDiagnostics WantDiags, llvm::unique_function)> OnUpdated) { auto Task = [=](decltype(OnUpdated) OnUpdated) mutable { + // Will be used to check if we can avoid rebuilding the AST. + bool InputsAreTheSame = + std::tie(FileInputs.CompileCommand, FileInputs.Contents) == + std::tie(Inputs.CompileCommand, Inputs.Contents); + tooling::CompileCommand OldCommand = std::move(FileInputs.CompileCommand); FileInputs = Inputs; // Remove the old AST if it's still in cache. @@ -343,16 +348,35 @@ return; } - std::shared_ptr NewPreamble = buildPreamble( - FileName, *Invocation, getPossiblyStalePreamble(), OldCommand, Inputs, - PCHs, StorePreambleInMemory, PreambleCallback); + std::shared_ptr OldPreamble = + getPossiblyStalePreamble(); + std::shared_ptr NewPreamble = + buildPreamble(FileName, *Invocation, OldPreamble, OldCommand, Inputs, + PCHs, StorePreambleInMemory, PreambleCallback); + + bool CanReuseAST = InputsAreTheSame && OldPreamble == NewPreamble; { std::lock_guard Lock(Mutex); + OldPreamble.reset(); if (NewPreamble) LastBuiltPreamble = NewPreamble; } PreambleWasBuilt.notify(); + if (CanReuseAST) { + // Take a shortcut and don't build the AST, neither the inputs nor the + // preamble have changed. + // Note that we do not report the diagnostics, since they should not have + // changed either. All the clients should handle the lack of OnUpdated() + // call anyway, to handle empty result from buildAST. + // FIXME(ibiryukov): the AST could actually change if non-preamble + // includes changed, but we choose to ignore it. + // FIXME(ibiryukov): should we refresh the cache in IdleASTs for the + // current file at this point? + vlog("Skipping rebuild of the AST for {0}, inputs are the same.", + FileName); + return; + } // Build the AST for diagnostics. llvm::Optional AST = buildAST(FileName, std::move(Invocation), Inputs, NewPreamble, PCHs); Index: test/clangd/extra-flags.test =================================================================== --- test/clangd/extra-flags.test +++ test/clangd/extra-flags.test @@ -23,7 +23,7 @@ # CHECK-NEXT: "uri": "file://{{.*}}/foo.c" # CHECK-NEXT: } --- -{"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"test:///foo.c","version":2},"contentChanges":[{"text":"int main() { int i; return i; }"}]}} +{"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"test:///foo.c","version":2},"contentChanges":[{"text":"int main() { int i; return i+1; }"}]}} # CHECK: "method": "textDocument/publishDiagnostics", # CHECK-NEXT: "params": { # CHECK-NEXT: "diagnostics": [ Index: unittests/clangd/TUSchedulerTests.cpp =================================================================== --- unittests/clangd/TUSchedulerTests.cpp +++ unittests/clangd/TUSchedulerTests.cpp @@ -33,13 +33,12 @@ class TUSchedulerTests : public ::testing::Test { protected: ParseInputs getInputs(PathRef File, std::string Contents) { - return ParseInputs{*CDB.getCompileCommand(File), buildTestFS(Files), - std::move(Contents)}; + return ParseInputs{*CDB.getCompileCommand(File), + buildTestFS(Files, Timestamps), std::move(Contents)}; } llvm::StringMap Files; - -private: + llvm::StringMap Timestamps; MockCompilationDatabase CDB; }; @@ -263,6 +262,10 @@ int* a; double* b = a; )cpp"; + llvm::StringLiteral OtherSourceContents = R"cpp( + int* a; + double* b = a + 0; + )cpp"; auto Foo = testPath("foo.cpp"); auto Bar = testPath("bar.cpp"); @@ -288,7 +291,7 @@ ASSERT_THAT(S.getFilesWithCachedAST(), UnorderedElementsAre(Bar, Baz)); // Access the old file again. - S.update(Foo, getInputs(Foo, SourceContents), WantDiagnostics::Yes, + S.update(Foo, getInputs(Foo, OtherSourceContents), WantDiagnostics::Yes, [&BuiltASTCounter](std::vector Diags) { ++BuiltASTCounter; }); ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1))); ASSERT_EQ(BuiltASTCounter.load(), 4); @@ -334,5 +337,58 @@ ASSERT_THAT(Preambles, Each(Preambles[0])); } +TEST_F(TUSchedulerTests, NoopOnEmptyChanges) { + TUScheduler S( + /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(), + /*StorePreambleInMemory=*/true, PreambleParsedCallback(), + /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), + ASTRetentionPolicy()); + + auto Source = testPath("foo.cpp"); + auto Header = testPath("foo.h"); + + Files[Header] = "int a;"; + Timestamps[Header] = time_t(0); + + auto SourceContents = R"cpp( + #include "foo.h" + int b = a; + )cpp"; + + // Return value indicates if the updated callback was received. + auto DoUpdate = [&](ParseInputs Inputs) -> bool { + std::atomic Updated(false); + Updated = false; + S.update(Source, std::move(Inputs), WantDiagnostics::Yes, + [&Updated](std::vector) { Updated = true; }); + bool UpdateFinished = S.blockUntilIdle(timeoutSeconds(1)); + if (!UpdateFinished) + ADD_FAILURE() << "Updated has not finished in one second. Threading bug?"; + return Updated; + }; + + // Test that subsequent updates with the same inputs do not cause rebuilds. + ASSERT_TRUE(DoUpdate(getInputs(Source, SourceContents))); + ASSERT_FALSE(DoUpdate(getInputs(Source, SourceContents))); + + // Update to a header should cause a rebuild, though. + Files[Header] = time_t(1); + ASSERT_TRUE(DoUpdate(getInputs(Source, SourceContents))); + ASSERT_FALSE(DoUpdate(getInputs(Source, SourceContents))); + + // Update to the contents should cause a rebuild. + auto OtherSourceContents = R"cpp( + #include "foo.h" + int c = d; + )cpp"; + ASSERT_TRUE(DoUpdate(getInputs(Source, OtherSourceContents))); + ASSERT_FALSE(DoUpdate(getInputs(Source, OtherSourceContents))); + + // Update to the compile commands should also cause a rebuild. + CDB.ExtraClangFlags.push_back("-DSOMETHING"); + ASSERT_TRUE(DoUpdate(getInputs(Source, OtherSourceContents))); + ASSERT_FALSE(DoUpdate(getInputs(Source, OtherSourceContents))); +} + } // namespace clangd } // namespace clang Index: unittests/clangd/TestFS.h =================================================================== --- unittests/clangd/TestFS.h +++ unittests/clangd/TestFS.h @@ -23,7 +23,8 @@ // Builds a VFS that provides access to the provided files, plus temporary // directories. llvm::IntrusiveRefCntPtr -buildTestFS(llvm::StringMap const &Files); +buildTestFS(llvm::StringMap const &Files, + llvm::StringMap const &Timestamps = {}); // A VFS provider that returns TestFSes containing a provided set of files. class MockFSProvider : public FileSystemProvider { Index: unittests/clangd/TestFS.cpp =================================================================== --- unittests/clangd/TestFS.cpp +++ unittests/clangd/TestFS.cpp @@ -19,13 +19,15 @@ using namespace llvm; IntrusiveRefCntPtr -buildTestFS(StringMap const &Files) { +buildTestFS(llvm::StringMap const &Files, + llvm::StringMap const &Timestamps) { IntrusiveRefCntPtr MemFS( new vfs::InMemoryFileSystem); for (auto &FileAndContents : Files) { - MemFS->addFile(FileAndContents.first(), time_t(), - MemoryBuffer::getMemBufferCopy(FileAndContents.second, - FileAndContents.first())); + StringRef File = FileAndContents.first(); + MemFS->addFile( + File, Timestamps.lookup(File), + MemoryBuffer::getMemBufferCopy(FileAndContents.second, File)); } return MemFS; }