[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

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);

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": [

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

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;
 }

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 {