[clangd] Do not rebuild AST if inputs have not changed

Summary:
If the contents are the same, the update most likely comes from the
fact that compile commands were invalidated. In that case we want to
avoid rebuilds in case the compile commands are actually the same.

Reviewers: ioeric

Reviewed By: ioeric

Subscribers: simark, javed.absar, MaskRay, jkorous, arphaman, jfb, cfe-commits

Differential Revision: https://reviews.llvm.org/D49783

llvm-svn: 338012

diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp
index 8661763..36cada5 100644 (file)
--- a/clang-tools-extra/clangd/TUScheduler.cpp
+++ b/clang-tools-extra/clangd/TUScheduler.cpp
@@ -324,6 +324,11 @@ void ASTWorker::update(
     ParseInputs Inputs, WantDiagnostics WantDiags,
     llvm::unique_function<void(std::vector<Diag>)> 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,38 @@ void ASTWorker::update(
       return;
     }
 
-    std::shared_ptr<const PreambleData> NewPreamble = buildPreamble(
-        FileName, *Invocation, getPossiblyStalePreamble(), OldCommand, Inputs,
-        PCHs, StorePreambleInMemory, PreambleCallback);
+    std::shared_ptr<const PreambleData> OldPreamble =
+        getPossiblyStalePreamble();
+    std::shared_ptr<const PreambleData> NewPreamble =
+        buildPreamble(FileName, *Invocation, OldPreamble, OldCommand, Inputs,
+                      PCHs, StorePreambleInMemory, PreambleCallback);
+
+    bool CanReuseAST = InputsAreTheSame && (OldPreamble == NewPreamble);
     {
       std::lock_guard<std::mutex> Lock(Mutex);
       if (NewPreamble)
         LastBuiltPreamble = NewPreamble;
     }
+    // Before doing the expensive AST reparse, we want to release our reference
+    // to the old preamble, so it can be freed if there are no other references
+    // to it.
+    OldPreamble.reset();
     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?
+      log("Skipping rebuild of the AST for {0}, inputs are the same.",
+          FileName);
+      return;
+    }
     // Build the AST for diagnostics.
     llvm::Optional<ParsedAST> AST =
         buildAST(FileName, std::move(Invocation), Inputs, NewPreamble, PCHs);

diff --git a/clang-tools-extra/test/clangd/extra-flags.test b/clang-tools-extra/test/clangd/extra-flags.test
index 23b2c65..f360cca 100644 (file)
--- a/clang-tools-extra/test/clangd/extra-flags.test
+++ b/clang-tools-extra/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": [

diff --git a/clang-tools-extra/unittests/clangd/TUSchedulerTests.cpp b/clang-tools-extra/unittests/clangd/TUSchedulerTests.cpp
index f345f9c..6a2851c 100644 (file)
--- a/clang-tools-extra/unittests/clangd/TUSchedulerTests.cpp
+++ b/clang-tools-extra/unittests/clangd/TUSchedulerTests.cpp
@@ -33,13 +33,12 @@ void ignoreError(llvm::Error Err) {
 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<std::string> Files;
-
-private:
+  llvm::StringMap<time_t> Timestamps;
   MockCompilationDatabase CDB;
 };
 
@@ -263,6 +262,10 @@ TEST_F(TUSchedulerTests, EvictedAST) {
     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 @@ TEST_F(TUSchedulerTests, EvictedAST) {
   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<Diag> Diags) { ++BuiltASTCounter; });
   ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
   ASSERT_EQ(BuiltASTCounter.load(), 4);
@@ -334,5 +337,58 @@ TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
   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<bool> Updated(false);
+    Updated = false;
+    S.update(Source, std::move(Inputs), WantDiagnostics::Yes,
+             [&Updated](std::vector<Diag>) { 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

diff --git a/clang-tools-extra/unittests/clangd/TestFS.cpp b/clang-tools-extra/unittests/clangd/TestFS.cpp
index 741eb8c..b3081d6 100644 (file)
--- a/clang-tools-extra/unittests/clangd/TestFS.cpp
+++ b/clang-tools-extra/unittests/clangd/TestFS.cpp
@@ -19,13 +19,15 @@ namespace clangd {
 using namespace llvm;
 
 IntrusiveRefCntPtr<vfs::FileSystem>
-buildTestFS(StringMap<std::string> const &Files) {
+buildTestFS(llvm::StringMap<std::string> const &Files,
+            llvm::StringMap<time_t> const &Timestamps) {
   IntrusiveRefCntPtr<vfs::InMemoryFileSystem> 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;
 }

diff --git a/clang-tools-extra/unittests/clangd/TestFS.h b/clang-tools-extra/unittests/clangd/TestFS.h
index be4aac4..9c2a15e 100644 (file)
--- a/clang-tools-extra/unittests/clangd/TestFS.h
+++ b/clang-tools-extra/unittests/clangd/TestFS.h
@@ -23,7 +23,8 @@ namespace clangd {
 // Builds a VFS that provides access to the provided files, plus temporary
 // directories.
 llvm::IntrusiveRefCntPtr<vfs::FileSystem>
-buildTestFS(llvm::StringMap<std::string> const &Files);
+buildTestFS(llvm::StringMap<std::string> const &Files,
+            llvm::StringMap<time_t> const &Timestamps = {});
 
 // A VFS provider that returns TestFSes containing a provided set of files.
 class MockFSProvider : public FileSystemProvider {