[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc31367e95ce1: [clangd] Build ASTs only with fresh preambles 
or after building a new preamble (authored by kadircet).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76725/new/

https://reviews.llvm.org/D76725

Files:
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp

Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -35,7 +35,7 @@
   Files[ImportThunk] = ThunkContents;
 
   ParseInputs Inputs;
-  auto& Argv = Inputs.CompileCommand.CommandLine;
+  auto &Argv = Inputs.CompileCommand.CommandLine;
   Argv = {"clang"};
   // FIXME: this shouldn't need to be conditional, but it breaks a
   // GoToDefinition test for some reason (getMacroArgExpandedLocation fails).
@@ -71,8 +71,7 @@
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
   auto Preamble =
-  buildPreamble(testPath(Filename), *CI,
-/*OldPreamble=*/nullptr, Inputs,
+  buildPreamble(testPath(Filename), *CI, Inputs,
 /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
   auto AST = buildAST(testPath(Filename), std::move(CI), Diags.take(), Inputs,
   Preamble);
@@ -89,7 +88,7 @@
 if (llvm::StringRef(Code).contains(Marker) ||
 llvm::StringRef(HeaderCode).contains(Marker))
   return true;
-for (const auto& KV : this->AdditionalFiles)
+for (const auto &KV : this->AdditionalFiles)
   if (llvm::StringRef(KV.second).contains(Marker))
 return true;
 return false;
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -24,6 +24,7 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -283,6 +284,7 @@
 S.runWithPreamble("StaleRead", Path, TUScheduler::Stale,
   [&](Expected Pre) {
 ASSERT_TRUE(bool(Pre));
+ASSERT_TRUE(Pre->Preamble);
 EXPECT_EQ(Pre->Preamble->Version, "A");
 EXPECT_THAT(includes(Pre->Preamble),
 ElementsAre(""));
@@ -292,11 +294,13 @@
 S.runWithPreamble("ConsistentRead", Path, TUScheduler::Consistent,
   [&](Expected Pre) {
 ASSERT_TRUE(bool(Pre));
+ASSERT_TRUE(Pre->Preamble);
 EXPECT_EQ(Pre->Preamble->Version, "B");
 EXPECT_THAT(includes(Pre->Preamble),
 ElementsAre(""));
 ++CallbackCount;
   });
+S.blockUntilIdle(timeoutSeconds(10));
   }
   EXPECT_EQ(2, CallbackCount);
 }
@@ -731,11 +735,14 @@
 )cpp";
 
   ParseInputs Inputs = getInputs(Source, SourceContents);
+  std::atomic DiagCount(0);
 
   // Update the source contents, which should trigger an initial build with
   // the header file missing.
   updateWithDiags(
-  S, Source, Inputs, WantDiagnostics::Yes, [](std::vector Diags) {
+  S, Source, Inputs, WantDiagnostics::Yes,
+  [&DiagCount](std::vector Diags) {
+++DiagCount;
 EXPECT_THAT(Diags,
 ElementsAre(Field(&Diag::Message, "'foo.h' file not found"),
 Field(&Diag::Message,
@@ -751,18 +758,23 @@
   // The addition of the missing header file shouldn't trigger a rebuild since
   // we don't track missing files.
   updateWithDiags(
-  S, Source, Inputs, WantDiagnostics::Yes, [](std::vector Diags) {
+  S, Source, Inputs, WantDiagnostics::Yes,
+  [&DiagCount](std::vector Diags) {
+++DiagCount;
 ADD_FAILURE() << "Did not expect diagnostics for missing header update";
   });
 
   // Forcing the reload should should cause a rebuild which no longer has any
   // errors.
   Inputs.ForceRebuild = true;
-  updateWithDiags(
-  S, Source, Inputs, WantDiagnostics::Yes,
-  [](std::vector Diags) { EXPECT_THAT(Diags, IsEmpty()); });
+  updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
+  [&DiagCount](std::vector Diags) {
+++DiagCount;
+EXPECT_THAT(Diags, IsEmpty());
+  });
 
   ASSERT_TRU

[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 255425.
kadircet marked 10 inline comments as done.
kadircet added a comment.

- Address comments and rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76725/new/

https://reviews.llvm.org/D76725

Files:
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp

Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -35,7 +35,7 @@
   Files[ImportThunk] = ThunkContents;
 
   ParseInputs Inputs;
-  auto& Argv = Inputs.CompileCommand.CommandLine;
+  auto &Argv = Inputs.CompileCommand.CommandLine;
   Argv = {"clang"};
   // FIXME: this shouldn't need to be conditional, but it breaks a
   // GoToDefinition test for some reason (getMacroArgExpandedLocation fails).
@@ -71,8 +71,7 @@
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
   auto Preamble =
-  buildPreamble(testPath(Filename), *CI,
-/*OldPreamble=*/nullptr, Inputs,
+  buildPreamble(testPath(Filename), *CI, Inputs,
 /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
   auto AST = buildAST(testPath(Filename), std::move(CI), Diags.take(), Inputs,
   Preamble);
@@ -89,7 +88,7 @@
 if (llvm::StringRef(Code).contains(Marker) ||
 llvm::StringRef(HeaderCode).contains(Marker))
   return true;
-for (const auto& KV : this->AdditionalFiles)
+for (const auto &KV : this->AdditionalFiles)
   if (llvm::StringRef(KV.second).contains(Marker))
 return true;
 return false;
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -24,6 +24,7 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -283,6 +284,7 @@
 S.runWithPreamble("StaleRead", Path, TUScheduler::Stale,
   [&](Expected Pre) {
 ASSERT_TRUE(bool(Pre));
+ASSERT_TRUE(Pre->Preamble);
 EXPECT_EQ(Pre->Preamble->Version, "A");
 EXPECT_THAT(includes(Pre->Preamble),
 ElementsAre(""));
@@ -292,11 +294,13 @@
 S.runWithPreamble("ConsistentRead", Path, TUScheduler::Consistent,
   [&](Expected Pre) {
 ASSERT_TRUE(bool(Pre));
+ASSERT_TRUE(Pre->Preamble);
 EXPECT_EQ(Pre->Preamble->Version, "B");
 EXPECT_THAT(includes(Pre->Preamble),
 ElementsAre(""));
 ++CallbackCount;
   });
+S.blockUntilIdle(timeoutSeconds(10));
   }
   EXPECT_EQ(2, CallbackCount);
 }
@@ -731,11 +735,14 @@
 )cpp";
 
   ParseInputs Inputs = getInputs(Source, SourceContents);
+  std::atomic DiagCount(0);
 
   // Update the source contents, which should trigger an initial build with
   // the header file missing.
   updateWithDiags(
-  S, Source, Inputs, WantDiagnostics::Yes, [](std::vector Diags) {
+  S, Source, Inputs, WantDiagnostics::Yes,
+  [&DiagCount](std::vector Diags) {
+++DiagCount;
 EXPECT_THAT(Diags,
 ElementsAre(Field(&Diag::Message, "'foo.h' file not found"),
 Field(&Diag::Message,
@@ -751,18 +758,23 @@
   // The addition of the missing header file shouldn't trigger a rebuild since
   // we don't track missing files.
   updateWithDiags(
-  S, Source, Inputs, WantDiagnostics::Yes, [](std::vector Diags) {
+  S, Source, Inputs, WantDiagnostics::Yes,
+  [&DiagCount](std::vector Diags) {
+++DiagCount;
 ADD_FAILURE() << "Did not expect diagnostics for missing header update";
   });
 
   // Forcing the reload should should cause a rebuild which no longer has any
   // errors.
   Inputs.ForceRebuild = true;
-  updateWithDiags(
-  S, Source, Inputs, WantDiagnostics::Yes,
-  [](std::vector Diags) { EXPECT_THAT(Diags, IsEmpty()); });
+  updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
+  [&DiagCount](std::vector Diags) {
+++DiagCount;
+EXPECT_THAT(Diags, IsEmpty());
+  });
 
   ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+  EXPECT_EQ(DiagCoun

[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked an inline comment as done.
kadircet added inline comments.



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:224
+  std::vector CIDiags, WantDiagnostics WantDiags) {
 // Make possibly expensive copy while not holding the lock.
+Request Req = {std::move(CI), std::move(PI), std::move(CIDiags), WantDiags,

sammccall wrote:
> comment is now obsolete, remove it and consider inlining `Req`
i would rather keep it out-of-line since it is used in two places.



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:325
   mutable std::condition_variable ReqCV;   /* GUARDED_BY(Mutex) */
   std::shared_ptr LatestBuild; /* GUARDED_BY(Mutex) */
 

sammccall wrote:
> this is no longer locked or concurrently accessed right?
right, deleted annottation and added a comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76725/new/

https://reviews.llvm.org/D76725



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thanks for your patience!
This is very nice, only +100 lines on TUScheduler.cpp in the end!




Comment at: clang-tools-extra/clangd/TUScheduler.cpp:224
+  std::vector CIDiags, WantDiagnostics WantDiags) {
 // Make possibly expensive copy while not holding the lock.
+Request Req = {std::move(CI), std::move(PI), std::move(CIDiags), WantDiags,

comment is now obsolete, remove it and consider inlining `Req`



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:325
   mutable std::condition_variable ReqCV;   /* GUARDED_BY(Mutex) */
   std::shared_ptr LatestBuild; /* GUARDED_BY(Mutex) */
 

this is no longer locked or concurrently accessed right?



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:378
+  /// Used to inform ASTWorker about a new preamble build by PreambleThread.
+  /// Diagnostics are only published through this callback, which ensures they
+  /// are always for newer versions of the file. As this callback gets called 
in

nit: last sentence is a fragment, I think you want to swap the dot and comma:

...through this callback. This ensures... of the file, as the callback...



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:408
+
+  /// Returns the latest built preamble, might be null if no preamble has been
+  /// built or latest attempt resulted in a failure.

Unused, I think?



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:793
+  // Used to check whether we can update AST cache.
+  bool InputsAreTheSame =
+  std::tie(FileInputs->CompileCommand, FileInputs->Contents) ==

I'd suggest InputsAreLatest or something that hints at *why* they might differ



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:810
+  });
+  llvm::Optional> AST = IdleASTs.take(this);
+  if (!AST) {

This is reusing the AST built for a read if the file (and preamble) hasn't 
changed, right?

I think it's helpful to explain where teh cache data might be coming from.

IIRC this is the case we thought wasn't so important. It competes with a 
different optimization: skipping clang-tidy, warning analysis, etc when 
building ASTs because we know their diagnostics will never be used.

This might be a good place to leave a comment like "FIXME: maybe we're better 
off never reusing this AST, so queued AST builds aren't required to produce 
diagnostics" or so



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:862
+  // Stash the AST in the cache for further use if it was build for current
+  // inputs.
+  if (InputsAreTheSame) {

This explains the what but not the why.
(We may be building an older version of the source, as the queue raced ahead 
while we were waiting on the preamble. In that case the queue can't reuse the 
AST we built.)
Comment could go here or on InputsAreTheSame


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76725/new/

https://reviews.llvm.org/D76725



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 254520.
kadircet added a comment.

- Rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76725/new/

https://reviews.llvm.org/D76725

Files:
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp

Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -35,7 +35,7 @@
   Files[ImportThunk] = ThunkContents;
 
   ParseInputs Inputs;
-  auto& Argv = Inputs.CompileCommand.CommandLine;
+  auto &Argv = Inputs.CompileCommand.CommandLine;
   Argv = {"clang"};
   // FIXME: this shouldn't need to be conditional, but it breaks a
   // GoToDefinition test for some reason (getMacroArgExpandedLocation fails).
@@ -71,8 +71,7 @@
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
   auto Preamble =
-  buildPreamble(testPath(Filename), *CI,
-/*OldPreamble=*/nullptr, Inputs,
+  buildPreamble(testPath(Filename), *CI, Inputs,
 /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
   auto AST = buildAST(testPath(Filename), std::move(CI), Diags.take(), Inputs,
   Preamble);
@@ -89,7 +88,7 @@
 if (llvm::StringRef(Code).contains(Marker) ||
 llvm::StringRef(HeaderCode).contains(Marker))
   return true;
-for (const auto& KV : this->AdditionalFiles)
+for (const auto &KV : this->AdditionalFiles)
   if (llvm::StringRef(KV.second).contains(Marker))
 return true;
 return false;
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -24,6 +24,7 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -283,6 +284,7 @@
 S.runWithPreamble("StaleRead", Path, TUScheduler::Stale,
   [&](Expected Pre) {
 ASSERT_TRUE(bool(Pre));
+ASSERT_TRUE(Pre->Preamble);
 EXPECT_EQ(Pre->Preamble->Version, "A");
 EXPECT_THAT(includes(Pre->Preamble),
 ElementsAre(""));
@@ -292,11 +294,13 @@
 S.runWithPreamble("ConsistentRead", Path, TUScheduler::Consistent,
   [&](Expected Pre) {
 ASSERT_TRUE(bool(Pre));
+ASSERT_TRUE(Pre->Preamble);
 EXPECT_EQ(Pre->Preamble->Version, "B");
 EXPECT_THAT(includes(Pre->Preamble),
 ElementsAre(""));
 ++CallbackCount;
   });
+S.blockUntilIdle(timeoutSeconds(10));
   }
   EXPECT_EQ(2, CallbackCount);
 }
@@ -731,11 +735,14 @@
 )cpp";
 
   ParseInputs Inputs = getInputs(Source, SourceContents);
+  std::atomic DiagCount(0);
 
   // Update the source contents, which should trigger an initial build with
   // the header file missing.
   updateWithDiags(
-  S, Source, Inputs, WantDiagnostics::Yes, [](std::vector Diags) {
+  S, Source, Inputs, WantDiagnostics::Yes,
+  [&DiagCount](std::vector Diags) {
+++DiagCount;
 EXPECT_THAT(Diags,
 ElementsAre(Field(&Diag::Message, "'foo.h' file not found"),
 Field(&Diag::Message,
@@ -751,18 +758,23 @@
   // The addition of the missing header file shouldn't trigger a rebuild since
   // we don't track missing files.
   updateWithDiags(
-  S, Source, Inputs, WantDiagnostics::Yes, [](std::vector Diags) {
+  S, Source, Inputs, WantDiagnostics::Yes,
+  [&DiagCount](std::vector Diags) {
+++DiagCount;
 ADD_FAILURE() << "Did not expect diagnostics for missing header update";
   });
 
   // Forcing the reload should should cause a rebuild which no longer has any
   // errors.
   Inputs.ForceRebuild = true;
-  updateWithDiags(
-  S, Source, Inputs, WantDiagnostics::Yes,
-  [](std::vector Diags) { EXPECT_THAT(Diags, IsEmpty()); });
+  updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
+  [&DiagCount](std::vector Diags) {
+++DiagCount;
+EXPECT_THAT(Diags, IsEmpty());
+  });
 
   ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+  EXPECT_EQ(DiagCount, 2U);
 }
 TEST_F(TUSchedulerTests, NoChangeDiags) {
   TUSchedu

[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

preambleReady part is a little bit different than you've described, it looks 
more like:

  ASTWorker::preambleReady(): enqueue({
if(preamble!=lastPreamble) {
   lastPreamble = preamble
   cache.get(); // force next read to use this preamble
}
build ast
publish ast.diagnostics
if (inputs == currentInputs) cache.put(ast)
  })

also the last 3 steps are on a different function called `generateDiagnostics` 
since the code is a little bit long.
it immediately follows the `preambleReady`(now named  `updatePreamble`).




Comment at: clang-tools-extra/clangd/TUScheduler.cpp:17
+// An update request changes latest inputs to ensure any subsequent read sees
+// the version of the file they were requested. In addition to that it might
+// result in publishing diagnostics.

sammccall wrote:
> You need to mention "building an AST" here, as you reference it below.
just saying "it will issue a build for new inputs", which is explained in 
details in the next paragraph.



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:29
+// modification to relevant part of the current file. Such a preamble is called
+// usable.
+// In the presence of stale(non-usable) preambles, ASTWorker won't publish

sammccall wrote:
> I don't think "usable" is a good name for this, because we *use* preambles 
> that are not "usable".
> 
> I think "compatible" or "up-to-date" or "fresh" or so would have enough 
> semantic distance to make this less confusing.
using compatible.



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:341
   SynchronizedTUStatus &Status;
+  ASTWorker &AW;
 };

sammccall wrote:
> Please don't use initialisms like this for members, they're hard to follow 
> out of context.
> I'd suggest ASTPeer (and PreamblePeer) or just Peer, to indicate these 
> threads are tightly-coupled partners.
PW was for PeerWorker :P



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:554
+  LatestBuild->Version, Inputs.Version, FileName);
+  // We still notify the ASTWorker to make sure diagnostics are updated to
+  // the latest version of the file without requiring user update.

sammccall wrote:
> This is going to trigger a rebuild of the fileindex - is it really necessary?
astworker won't trigger that if inputs are the same.



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:630
+  WantDiagnostics WantDiags) {
+  std::string TaskName = llvm::formatv("Preamble Update ({0})", PI.Version);
+  // Store preamble and build diagnostics with new preamble if requested.

sammccall wrote:
> "Golden AST from preamble"?
> Current text is not terribly descriptive...
this is not necessarily a golden ast now. just changing it to "Build AST"



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:675
+  assert(LatestPreamble);
+  assert(isPreambleUsable(*LatestPreamble, Inputs, FileName, *Invocation));
+

sammccall wrote:
> this isn't a safe assert - it does IO and could transiently become true/false
right, thanks for catching it!

the one above is also not true (`assert(LatestPreamble)`) as preamble build 
might've failed. I suppose we would still want to move forward so that we can 
surface diagnostics saying `why it failed`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76725/new/

https://reviews.llvm.org/D76725



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 254439.
kadircet marked 31 inline comments as done.
kadircet added a comment.

- Update comments and re-order functions


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76725/new/

https://reviews.llvm.org/D76725

Files:
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp

Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -35,7 +35,7 @@
   Files[ImportThunk] = ThunkContents;
 
   ParseInputs Inputs;
-  auto& Argv = Inputs.CompileCommand.CommandLine;
+  auto &Argv = Inputs.CompileCommand.CommandLine;
   Argv = {"clang"};
   // FIXME: this shouldn't need to be conditional, but it breaks a
   // GoToDefinition test for some reason (getMacroArgExpandedLocation fails).
@@ -71,8 +71,7 @@
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
   auto Preamble =
-  buildPreamble(testPath(Filename), *CI,
-/*OldPreamble=*/nullptr, Inputs,
+  buildPreamble(testPath(Filename), *CI, Inputs,
 /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
   auto AST = buildAST(testPath(Filename), std::move(CI), Diags.take(), Inputs,
   Preamble);
@@ -89,7 +88,7 @@
 if (llvm::StringRef(Code).contains(Marker) ||
 llvm::StringRef(HeaderCode).contains(Marker))
   return true;
-for (const auto& KV : this->AdditionalFiles)
+for (const auto &KV : this->AdditionalFiles)
   if (llvm::StringRef(KV.second).contains(Marker))
 return true;
 return false;
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -24,6 +24,7 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -283,6 +284,7 @@
 S.runWithPreamble("StaleRead", Path, TUScheduler::Stale,
   [&](Expected Pre) {
 ASSERT_TRUE(bool(Pre));
+ASSERT_TRUE(Pre->Preamble);
 EXPECT_EQ(Pre->Preamble->Version, "A");
 EXPECT_THAT(includes(Pre->Preamble),
 ElementsAre(""));
@@ -292,11 +294,13 @@
 S.runWithPreamble("ConsistentRead", Path, TUScheduler::Consistent,
   [&](Expected Pre) {
 ASSERT_TRUE(bool(Pre));
+ASSERT_TRUE(Pre->Preamble);
 EXPECT_EQ(Pre->Preamble->Version, "B");
 EXPECT_THAT(includes(Pre->Preamble),
 ElementsAre(""));
 ++CallbackCount;
   });
+S.blockUntilIdle(timeoutSeconds(10));
   }
   EXPECT_EQ(2, CallbackCount);
 }
@@ -731,11 +735,14 @@
 )cpp";
 
   ParseInputs Inputs = getInputs(Source, SourceContents);
+  std::atomic DiagCount(0);
 
   // Update the source contents, which should trigger an initial build with
   // the header file missing.
   updateWithDiags(
-  S, Source, Inputs, WantDiagnostics::Yes, [](std::vector Diags) {
+  S, Source, Inputs, WantDiagnostics::Yes,
+  [&DiagCount](std::vector Diags) {
+++DiagCount;
 EXPECT_THAT(Diags,
 ElementsAre(Field(&Diag::Message, "'foo.h' file not found"),
 Field(&Diag::Message,
@@ -751,18 +758,23 @@
   // The addition of the missing header file shouldn't trigger a rebuild since
   // we don't track missing files.
   updateWithDiags(
-  S, Source, Inputs, WantDiagnostics::Yes, [](std::vector Diags) {
+  S, Source, Inputs, WantDiagnostics::Yes,
+  [&DiagCount](std::vector Diags) {
+++DiagCount;
 ADD_FAILURE() << "Did not expect diagnostics for missing header update";
   });
 
   // Forcing the reload should should cause a rebuild which no longer has any
   // errors.
   Inputs.ForceRebuild = true;
-  updateWithDiags(
-  S, Source, Inputs, WantDiagnostics::Yes,
-  [](std::vector Diags) { EXPECT_THAT(Diags, IsEmpty()); });
+  updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
+  [&DiagCount](std::vector Diags) {
+++DiagCount;
+EXPECT_THAT(Diags, IsEmpty());
+  });
 
   ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+  EXPECT_

[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Had some discussion offline.

- having ASTWorker not worry about preamble validity before dispatching to 
preambleworker seems like a win
- for this, preambleworker needs to call preambleReady whether it's new or not, 
so ASTWorker can produce diagnostics
- AST reuse from diagnostics->request seems much more useful than the other way 
around (e.g. it reduces request latency), so don't bother with the latter. (And 
we can drop diagnostic computation in some cases)

This yields pseudocode like:

  ASTWorker::update(): enqueue({
currentInputs = inputs
preambleworker::update(inputs)
  })
  ASTWorker::runWithAST(): enqueue({
ast = cache.get()
if (!ast) patch preamble and build ast
action(ast)
cache.put(ast)
  })
  PreambleWorker::update(): enqueue({
if (!preamble.compatible(inputs))
  build preamble
ASTWorker::preambleReady(preamble)
  })
  ASTWorker::preambleReady(): enqueue({
preamble = p
build ast
publish ast.diagnostics
if (inputs == currentInputs) cache.put(ast)
else if (preamble != oldPreamble) cache.get() // force next read to use 
this preamble
  })

(I'm not sure how simple the actual code can be. I do think defining the 
methods in that order may help readability)




Comment at: clang-tools-extra/clangd/TUScheduler.cpp:8
 
//===--===//
-// For each file, managed by TUScheduler, we create a single ASTWorker that
-// manages an AST for that file. All operations that modify or read the AST are
-// run on a separate dedicated thread asynchronously in FIFO order.
+// TUScheduler stores a worker per active file. This worker is called ASTWorker
+// and manages updates(modifications to file contents) and reads(actions

nit: just "This ASTWorker manages updates..."



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:8
 
//===--===//
-// For each file, managed by TUScheduler, we create a single ASTWorker that
-// manages an AST for that file. All operations that modify or read the AST are
-// run on a separate dedicated thread asynchronously in FIFO order.
+// TUScheduler stores a worker per active file. This worker is called ASTWorker
+// and manages updates(modifications to file contents) and reads(actions

sammccall wrote:
> nit: just "This ASTWorker manages updates..."
uber-nit: I'd say the scheduler *manages* workers and the worker *processes* 
updates.
(Just to avoid mixing metaphors)



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:9
+// TUScheduler stores a worker per active file. This worker is called ASTWorker
+// and manages updates(modifications to file contents) and reads(actions
+// performed on preamble/AST) to the file.

nit: space before open parens (not a big deal, but occurs several times)



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:16
 //
-// The processing thread of the ASTWorker is also responsible for building the
-// preamble. However, unlike AST, the same preamble can be read concurrently, 
so
-// we run each of async preamble reads on its own thread.
+// An update request changes latest inputs to ensure any subsequent read sees
+// the version of the file they were requested. In addition to that it might

changes latest inputs -> replaces the current parser inputs

("changes" suggests some tricky mutation, and inputs isn't defined here)



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:17
+// An update request changes latest inputs to ensure any subsequent read sees
+// the version of the file they were requested. In addition to that it might
+// result in publishing diagnostics.

You need to mention "building an AST" here, as you reference it below.



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:25
 //
-// Rationale for cancelling updates.
-// LSP clients can send updates to clangd on each keystroke. Some files take
-// significant time to parse (e.g. a few seconds) and clangd can get starved by
-// the updates to those files. Therefore we try to process only the last 
update,
-// if possible.
-// Our current strategy to do that is the following:
-// - For each update we immediately schedule rebuild of the AST.
-// - Rebuild of the AST checks if it was cancelled before doing any actual 
work.
-//   If it was, it does not do an actual rebuild, only reports llvm::None to 
the
-//   callback
-// - When adding an update, we cancel the last update in the queue if it didn't
-//   have any reads.
-// There is probably a optimal ways to do that. One approach we might take is
-// the following:
-// - For each update we remember the pending inputs, but delay rebuild of the
-//   AST for some timeout.
-// - If subsequent updates come before rebuild was started, we repl

[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 253556.
kadircet added a comment.

- Add assertion to explicitly spell out scheduling for golden ASTs.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76725/new/

https://reviews.llvm.org/D76725

Files:
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp

Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -66,8 +66,7 @@
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
   auto Preamble =
-  buildPreamble(FullFilename, *CI,
-/*OldPreamble=*/nullptr, Inputs,
+  buildPreamble(FullFilename, *CI, Inputs,
 /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
   auto AST =
   buildAST(FullFilename, std::move(CI), Diags.take(), Inputs, Preamble);
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -283,6 +283,7 @@
 S.runWithPreamble("StaleRead", Path, TUScheduler::Stale,
   [&](Expected Pre) {
 ASSERT_TRUE(bool(Pre));
+ASSERT_TRUE(Pre->Preamble);
 EXPECT_EQ(Pre->Preamble->Version, "A");
 EXPECT_THAT(includes(Pre->Preamble),
 ElementsAre(""));
@@ -292,11 +293,13 @@
 S.runWithPreamble("ConsistentRead", Path, TUScheduler::Consistent,
   [&](Expected Pre) {
 ASSERT_TRUE(bool(Pre));
+ASSERT_TRUE(Pre->Preamble);
 EXPECT_EQ(Pre->Preamble->Version, "B");
 EXPECT_THAT(includes(Pre->Preamble),
 ElementsAre(""));
 ++CallbackCount;
   });
+S.blockUntilIdle(timeoutSeconds(10));
   }
   EXPECT_EQ(2, CallbackCount);
 }
@@ -853,15 +856,19 @@
   TUState(PreambleAction::Idle, ASTAction::RunningAction),
   // We build the preamble
   TUState(PreambleAction::Building, ASTAction::RunningAction),
-  // Preamble worker goes idle
+  // We built the preamble, and issued ast built on ASTWorker
+  // thread. Preambleworker goes idle afterwards.
   TUState(PreambleAction::Idle, ASTAction::RunningAction),
-  // We start building the ast
+  // Start task for building the ast, as a result of building
+  // preamble, on astworker thread.
+  TUState(PreambleAction::Idle, ASTAction::RunningAction),
+  // AST build starts.
   TUState(PreambleAction::Idle, ASTAction::Building),
-  // Built finished succesffully
+  // AST built finished successfully
   TUState(PreambleAction::Idle, ASTAction::Building),
-  // Rnning go to def
+  // Running go to def
   TUState(PreambleAction::Idle, ASTAction::RunningAction),
-  // both workers go idle
+  // ASTWorker goes idle.
   TUState(PreambleAction::Idle, ASTAction::Idle)));
 }
 
Index: clang-tools-extra/clangd/unittests/FileIndexTests.cpp
===
--- clang-tools-extra/clangd/unittests/FileIndexTests.cpp
+++ clang-tools-extra/clangd/unittests/FileIndexTests.cpp
@@ -286,7 +286,7 @@
 
   FileIndex Index;
   bool IndexUpdated = false;
-  buildPreamble(FooCpp, *CI, /*OldPreamble=*/nullptr, PI,
+  buildPreamble(FooCpp, *CI, PI,
 /*StoreInMemory=*/true,
 [&](ASTContext &Ctx, std::shared_ptr PP,
 const CanonicalIncludes &CanonIncludes) {
@@ -424,7 +424,7 @@
 }
 
 TEST(FileIndexTest, MergeMainFileSymbols) {
-  const char* CommonHeader = "void foo();";
+  const char *CommonHeader = "void foo();";
   TestTU Header = TestTU::withCode(CommonHeader);
   TestTU Cpp = TestTU::withCode("void foo() {}");
   Cpp.Filename = "foo.cpp";
Index: clang-tools-extra/clangd/TUScheduler.cpp
===
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -5,41 +5,55 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--

[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 253552.
kadircet added a comment.

- Make use of a separate queue for golden ASTs to prevent any races that might

occur due PreambleThread finishing multiple preambles before ASTWorker gets to
process others.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76725/new/

https://reviews.llvm.org/D76725

Files:
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp

Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -66,8 +66,7 @@
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
   auto Preamble =
-  buildPreamble(FullFilename, *CI,
-/*OldPreamble=*/nullptr, Inputs,
+  buildPreamble(FullFilename, *CI, Inputs,
 /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
   auto AST =
   buildAST(FullFilename, std::move(CI), Diags.take(), Inputs, Preamble);
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -283,6 +283,7 @@
 S.runWithPreamble("StaleRead", Path, TUScheduler::Stale,
   [&](Expected Pre) {
 ASSERT_TRUE(bool(Pre));
+ASSERT_TRUE(Pre->Preamble);
 EXPECT_EQ(Pre->Preamble->Version, "A");
 EXPECT_THAT(includes(Pre->Preamble),
 ElementsAre(""));
@@ -292,11 +293,13 @@
 S.runWithPreamble("ConsistentRead", Path, TUScheduler::Consistent,
   [&](Expected Pre) {
 ASSERT_TRUE(bool(Pre));
+ASSERT_TRUE(Pre->Preamble);
 EXPECT_EQ(Pre->Preamble->Version, "B");
 EXPECT_THAT(includes(Pre->Preamble),
 ElementsAre(""));
 ++CallbackCount;
   });
+S.blockUntilIdle(timeoutSeconds(10));
   }
   EXPECT_EQ(2, CallbackCount);
 }
@@ -853,15 +856,19 @@
   TUState(PreambleAction::Idle, ASTAction::RunningAction),
   // We build the preamble
   TUState(PreambleAction::Building, ASTAction::RunningAction),
-  // Preamble worker goes idle
+  // We built the preamble, and issued ast built on ASTWorker
+  // thread. Preambleworker goes idle afterwards.
   TUState(PreambleAction::Idle, ASTAction::RunningAction),
-  // We start building the ast
+  // Start task for building the ast, as a result of building
+  // preamble, on astworker thread.
+  TUState(PreambleAction::Idle, ASTAction::RunningAction),
+  // AST build starts.
   TUState(PreambleAction::Idle, ASTAction::Building),
-  // Built finished succesffully
+  // AST built finished successfully
   TUState(PreambleAction::Idle, ASTAction::Building),
-  // Rnning go to def
+  // Running go to def
   TUState(PreambleAction::Idle, ASTAction::RunningAction),
-  // both workers go idle
+  // ASTWorker goes idle.
   TUState(PreambleAction::Idle, ASTAction::Idle)));
 }
 
Index: clang-tools-extra/clangd/unittests/FileIndexTests.cpp
===
--- clang-tools-extra/clangd/unittests/FileIndexTests.cpp
+++ clang-tools-extra/clangd/unittests/FileIndexTests.cpp
@@ -286,7 +286,7 @@
 
   FileIndex Index;
   bool IndexUpdated = false;
-  buildPreamble(FooCpp, *CI, /*OldPreamble=*/nullptr, PI,
+  buildPreamble(FooCpp, *CI, PI,
 /*StoreInMemory=*/true,
 [&](ASTContext &Ctx, std::shared_ptr PP,
 const CanonicalIncludes &CanonIncludes) {
@@ -424,7 +424,7 @@
 }
 
 TEST(FileIndexTest, MergeMainFileSymbols) {
-  const char* CommonHeader = "void foo();";
+  const char *CommonHeader = "void foo();";
   TestTU Header = TestTU::withCode(CommonHeader);
   TestTU Cpp = TestTU::withCode("void foo() {}");
   Cpp.Filename = "foo.cpp";
Index: clang-tools-extra/clangd/TUScheduler.cpp
===
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUS

[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:623
   std::string TaskName = llvm::formatv("Update ({0})", Inputs.Version);
   auto Task = [=]() mutable {
 auto RunPublish = [&](llvm::function_ref Publish) {

sammccall wrote:
> sammccall wrote:
> > this function is getting too long/monolithic, we should find a way to split 
> > it up.
> > ASTTask should be a member function, I think.
> I also lost track of the control flow halfway through the function, and can't 
> work it out.
> I don't know really what to concretely advise, but it needs to be a lot 
> simpler, maybe by finding some better abstractions or just better 
> understanding conceptually what this is doing.
> ASTTask should be a member function, I think.

Moved it into a meber named `publishDiagnostics`



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:693
+std::lock_guard Lock(Mutex);
+// LatestPreamble might be the last reference to old preamble, do not
+// trigger destructor while holding the lock.

sammccall wrote:
> Didn't we decide to make this available only after building the golden AST 
> and publishing diagnostics?
as discussed offline, this is harmless since it is being executed on ASTWorker 
thread and makes new preamble available to code completion/signature help 
faster.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76725/new/

https://reviews.llvm.org/D76725



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 253347.
kadircet marked 26 inline comments as done.
kadircet added a comment.
Herald added a subscriber: jfb.

- Update description and address comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76725/new/

https://reviews.llvm.org/D76725

Files:
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp

Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -66,8 +66,7 @@
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
   auto Preamble =
-  buildPreamble(FullFilename, *CI,
-/*OldPreamble=*/nullptr, Inputs,
+  buildPreamble(FullFilename, *CI, Inputs,
 /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
   auto AST =
   buildAST(FullFilename, std::move(CI), Diags.take(), Inputs, Preamble);
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -283,6 +283,7 @@
 S.runWithPreamble("StaleRead", Path, TUScheduler::Stale,
   [&](Expected Pre) {
 ASSERT_TRUE(bool(Pre));
+ASSERT_TRUE(Pre->Preamble);
 EXPECT_EQ(Pre->Preamble->Version, "A");
 EXPECT_THAT(includes(Pre->Preamble),
 ElementsAre(""));
@@ -292,11 +293,13 @@
 S.runWithPreamble("ConsistentRead", Path, TUScheduler::Consistent,
   [&](Expected Pre) {
 ASSERT_TRUE(bool(Pre));
+ASSERT_TRUE(Pre->Preamble);
 EXPECT_EQ(Pre->Preamble->Version, "B");
 EXPECT_THAT(includes(Pre->Preamble),
 ElementsAre(""));
 ++CallbackCount;
   });
+S.blockUntilIdle(timeoutSeconds(10));
   }
   EXPECT_EQ(2, CallbackCount);
 }
@@ -853,15 +856,19 @@
   TUState(PreambleAction::Idle, ASTAction::RunningAction),
   // We build the preamble
   TUState(PreambleAction::Building, ASTAction::RunningAction),
-  // Preamble worker goes idle
+  // We built the preamble, and issued ast built on ASTWorker
+  // thread. Preambleworker goes idle afterwards.
   TUState(PreambleAction::Idle, ASTAction::RunningAction),
-  // We start building the ast
+  // Start task for building the ast, as a result of building
+  // preamble, on astworker thread.
+  TUState(PreambleAction::Idle, ASTAction::RunningAction),
+  // AST build starts.
   TUState(PreambleAction::Idle, ASTAction::Building),
-  // Built finished succesffully
+  // AST built finished successfully
   TUState(PreambleAction::Idle, ASTAction::Building),
-  // Rnning go to def
+  // Running go to def
   TUState(PreambleAction::Idle, ASTAction::RunningAction),
-  // both workers go idle
+  // ASTWorker goes idle.
   TUState(PreambleAction::Idle, ASTAction::Idle)));
 }
 
Index: clang-tools-extra/clangd/unittests/FileIndexTests.cpp
===
--- clang-tools-extra/clangd/unittests/FileIndexTests.cpp
+++ clang-tools-extra/clangd/unittests/FileIndexTests.cpp
@@ -286,7 +286,7 @@
 
   FileIndex Index;
   bool IndexUpdated = false;
-  buildPreamble(FooCpp, *CI, /*OldPreamble=*/nullptr, PI,
+  buildPreamble(FooCpp, *CI, PI,
 /*StoreInMemory=*/true,
 [&](ASTContext &Ctx, std::shared_ptr PP,
 const CanonicalIncludes &CanonIncludes) {
@@ -424,7 +424,7 @@
 }
 
 TEST(FileIndexTest, MergeMainFileSymbols) {
-  const char* CommonHeader = "void foo();";
+  const char *CommonHeader = "void foo();";
   TestTU Header = TestTU::withCode(CommonHeader);
   TestTU Cpp = TestTU::withCode("void foo() {}");
   Cpp.Filename = "foo.cpp";
Index: clang-tools-extra/clangd/TUScheduler.cpp
===
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -5,41 +5,52 @@
 // SPDX-License-Identifie

[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:8
 
//===--===//
 // For each file, managed by TUScheduler, we create a single ASTWorker that
+// manages an AST and a preamble for that file. All operations that modify or

This is explaining a pretty complicated thing, so I think it's particularly 
important to clearly organize the explanation, use consistent terminology, and 
avoid including unneccesary details.

I'd suggest introducing with paragraphs in this order:

- TUScheduler and the idea of a worker per TU.
- ASTWorker thread, the queue of interleaved updates and reads, elision of dead 
updates.
- Compatibility of preambles and updates, picky reads. 
- PreambleWorker thread, elision of preambles, golden ASTs.
- Published ASTs, epochs/monotonicity.



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:11
+// read the AST are run on a separate dedicated thread asynchronously in FIFO
+// order. There is a second thread for preamble builds, ASTWorker thread issues
+// updates on that preamble thread as preamble becomes stale.

nit: name the other thread (there is a second PreambleWorker thread ...)



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:12
+// order. There is a second thread for preamble builds, ASTWorker thread issues
+// updates on that preamble thread as preamble becomes stale.
+//

"issues updates" is vague. What about "... enqueues rebuilds on the 
PreambleWorker thread as preamble becomes stale. Currently, it then immediately 
blocks on that request."



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:19
+// FIXME: Get rid of the synchronization between ASTWorker::update and
+// PreambleThread builds by using patched ASTs instead of compatible preambles
+// for reads. Only keep the blocking behaviour for picky read requests, e.g.

You haven't defined "compatible" anywhere, and it's not obvious what it means.



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:23
+//
+// Updates are processed in two phases, by issuing a preamble and an ast build.
+// The preamble build gets issued into a different worker and might be

The wording here (particularly the word "phases") implies sequencing: an update 
results in a preamble build followed by an ast build in sequence, when in fact 
it usually results in an ast build and preamble build in parallel (the former 
usually finishing first) with a second ast build sequenced after the preamble 
build.



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:25
+// The preamble build gets issued into a different worker and might be
+// overwritten by subsequent updates. An AST will be built for an update if
+// latest built preamble is compatible for it or a new preamble gets built for

This isn't true, an AST is built for an update if it is needed (a read is based 
on it, wantdiagnostics=yes, it changed the preamble, or wantdiagnostics=maybe 
and the debounce expired)



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:29
+// AST builds are always executed on ASTWorker thread, either immediately
+// following an update request with a compatible preamble, or through a new
+// request, inserted at front of the queue, from PreambleThread after building 
a

I'm not sure what compatible means here, but we will certainly build ASTs for 
incompatible preambles.
(Or is this describing the intermediate state as of this patch, with the plan 
to rewrite it all next patch? I think we should rather describe the new model, 
and then document the current deviations from it)



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:29
+// AST builds are always executed on ASTWorker thread, either immediately
+// following an update request with a compatible preamble, or through a new
+// request, inserted at front of the queue, from PreambleThread after building 
a

sammccall wrote:
> I'm not sure what compatible means here, but we will certainly build ASTs for 
> incompatible preambles.
> (Or is this describing the intermediate state as of this patch, with the plan 
> to rewrite it all next patch? I think we should rather describe the new 
> model, and then document the current deviations from it)
I'm not sure what "immediately following" is relative to: if it's after the 
update() call, it's not immediate - it has to wait in the queue.



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:33
+//
+// Diagnostics and index is updated as a result of building AST for write
+// requests. Progress on those updates are ensured with golden ASTs, as they'll

"and *the* index *are* updated"... "building *the* AST"



Comment at: clang-tools-extra/cla

[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 252841.
kadircet added a comment.

- Invalidate cached AST in case of preamble/input changes when building golden 
ASTs


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76725/new/

https://reviews.llvm.org/D76725

Files:
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp

Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -66,8 +66,7 @@
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
   auto Preamble =
-  buildPreamble(FullFilename, *CI,
-/*OldPreamble=*/nullptr, Inputs,
+  buildPreamble(FullFilename, *CI, Inputs,
 /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
   auto AST =
   buildAST(FullFilename, std::move(CI), Diags.take(), Inputs, Preamble);
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -283,6 +283,7 @@
 S.runWithPreamble("StaleRead", Path, TUScheduler::Stale,
   [&](Expected Pre) {
 ASSERT_TRUE(bool(Pre));
+ASSERT_TRUE(Pre->Preamble);
 EXPECT_EQ(Pre->Preamble->Version, "A");
 EXPECT_THAT(includes(Pre->Preamble),
 ElementsAre(""));
@@ -292,11 +293,13 @@
 S.runWithPreamble("ConsistentRead", Path, TUScheduler::Consistent,
   [&](Expected Pre) {
 ASSERT_TRUE(bool(Pre));
+ASSERT_TRUE(Pre->Preamble);
 EXPECT_EQ(Pre->Preamble->Version, "B");
 EXPECT_THAT(includes(Pre->Preamble),
 ElementsAre(""));
 ++CallbackCount;
   });
+S.blockUntilIdle(timeoutSeconds(10));
   }
   EXPECT_EQ(2, CallbackCount);
 }
@@ -853,15 +856,19 @@
   TUState(PreambleAction::Idle, ASTAction::RunningAction),
   // We build the preamble
   TUState(PreambleAction::Building, ASTAction::RunningAction),
-  // Preamble worker goes idle
+  // We built the preamble, and issued ast built on ASTWorker
+  // thread. Preambleworker goes idle afterwards.
   TUState(PreambleAction::Idle, ASTAction::RunningAction),
-  // We start building the ast
+  // Start task for building the ast, as a result of building
+  // preamble, on astworker thread.
+  TUState(PreambleAction::Idle, ASTAction::RunningAction),
+  // AST build starts.
   TUState(PreambleAction::Idle, ASTAction::Building),
-  // Built finished succesffully
+  // AST built finished successfully
   TUState(PreambleAction::Idle, ASTAction::Building),
-  // Rnning go to def
+  // Running go to def
   TUState(PreambleAction::Idle, ASTAction::RunningAction),
-  // both workers go idle
+  // ASTWorker goes idle.
   TUState(PreambleAction::Idle, ASTAction::Idle)));
 }
 
Index: clang-tools-extra/clangd/unittests/FileIndexTests.cpp
===
--- clang-tools-extra/clangd/unittests/FileIndexTests.cpp
+++ clang-tools-extra/clangd/unittests/FileIndexTests.cpp
@@ -286,7 +286,7 @@
 
   FileIndex Index;
   bool IndexUpdated = false;
-  buildPreamble(FooCpp, *CI, /*OldPreamble=*/nullptr, PI,
+  buildPreamble(FooCpp, *CI, PI,
 /*StoreInMemory=*/true,
 [&](ASTContext &Ctx, std::shared_ptr PP,
 const CanonicalIncludes &CanonIncludes) {
@@ -424,7 +424,7 @@
 }
 
 TEST(FileIndexTest, MergeMainFileSymbols) {
-  const char* CommonHeader = "void foo();";
+  const char *CommonHeader = "void foo();";
   TestTU Header = TestTU::withCode(CommonHeader);
   TestTU Cpp = TestTU::withCode("void foo() {}");
   Cpp.Filename = "foo.cpp";
Index: clang-tools-extra/clangd/TUScheduler.cpp
===
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -6,16 +6,46 @@
 //
 //===-

[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D76725#1940282 , @sammccall wrote:

> So I think we need a careful description of the new model somewhere. Not so 
> much on specific changes/constraints parts of the code operate under, but 
> what it's trying to do.
>
> My best understanding is:
>
> - In general, read requests are processed in order by the ASTWorker, and get 
> exactly the version of the file when the request was received. That is, the 
> ASTworker queue interleaves updates and reads in the same order as LSP.
> - Preambles are built on the PreambleWorker for a certain version of the 
> file, and may be compatible or incompatible with subsequent versions. If a 
> read is "picky" it may block the ASTWorker queue waiting for a compatible 
> preamble, otherwise it will "patch up" an incompatible one.
> - Diagnostics and indexes are updated opportunistically using onMainAST when 
> an AST is built, but only if the used preamble is compatible. Publishing 
> diagnostics from patched preambles is not allowed, but we also do not block 
> waiting for an up-to-date preamble in order to generate publishable 
> diagnostics. (Exception: if WantDiagnostics=Yes, we block).
> - To ensure forward progress with diagnostics if the queue is full, when 
> preamble is built we immediately build a "golden" AST from that version and 
> publish diagnostics. To ensure diagnostics do not "go backwards", 
> opportunistic diagnostics are suppressed if they don't use the latest 
> preamble (the one with the last golden AST). This means the sequence of ASTs 
> producing diagnostics is neatly partitioned into "epochs" of the preambles, 
> the first in the epoch is always the golden version. (Exception: if 
> WantDiagnostics=no, diagnostics are not emitted for the golden AST). Because 
> these versions are built out-of-order (not interleaved with reads per LSP), 
> the golden AST is not cached and reused for reads.
>
>   Is this about right?


Yes that's most of what this patch does. Tried to explain/sum up those in file 
comments.

> Based on this, I do think the model would be much easier to reason about and 
> verify if the golden AST was built on the ASTWorker thread in a queued task, 
> rather than on the PreambleWorker thread in a callback. This means we 
> understand allowed sequencing by looking at the queue rather than reasoning 
> about mutexes/threads with multiple possible interleavings. By pushing the 
> golden AST task to the front of the queue, it's more explicit that this is a 
> priorities/scheduling decision. Following what happened in logs is probably 
> easier due to less interleaving.
> 
>   (The callback could enqueue the task, or the PreambleWorker could just 
> enqueue the task directly at that point - not sure the callback indirection 
> buys anything).

Done. It is still the callback that enqueues the task, but moved storage of 
latest build preamble from PreambleThread to ASTWorker to prevent a possible 
race between currently running update in astworker and build of preamble in 
preamblethread, as discussed offline.
This also enabled us to cache golden ASTs in case ASTWorker hasn't received any 
updates in between which gets rid of the additional AST build cost.

> One thing that seems complicated in the model is that the AST build needs to 
> decide whether to block on a compatible preamble, but it's the following read 
> that determines whether it needs to. In the worst case, the picky read hasn't 
> even been scheduled yet. I think picky reads probably need to block on the 
> target preamble and build their AST themselves if the cached one isn't 
> suitable. If they "miss" their preamble (i.e. preambleVersion >= readVersion 
> but the preamble isn't compatible) then I think we should fail the request 
> (i.e. picky requests may be invalidated by subsequent preamble edits).

Agreed, but this is not in the scope of this patch, will address that in the 
upcoming patches after dropping the synchronization between preamble and ast 
thread. In addition to that i am planning to ensure updates with 
`WantDiagnostics::Yes` gets build by making them non-overwritable in 
PreambleThread. That is any subsequent build requests will block (astworker 
thread) until update with `WantDiags::Yes` starts building.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76725/new/

https://reviews.llvm.org/D76725



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 252633.
kadircet added a comment.

- Build golden asts in ASTWorker thread, rather than preamble thread.
- Explain new model in file comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76725/new/

https://reviews.llvm.org/D76725

Files:
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp

Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -66,8 +66,7 @@
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
   auto Preamble =
-  buildPreamble(FullFilename, *CI,
-/*OldPreamble=*/nullptr, Inputs,
+  buildPreamble(FullFilename, *CI, Inputs,
 /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
   auto AST =
   buildAST(FullFilename, std::move(CI), Diags.take(), Inputs, Preamble);
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -283,6 +283,7 @@
 S.runWithPreamble("StaleRead", Path, TUScheduler::Stale,
   [&](Expected Pre) {
 ASSERT_TRUE(bool(Pre));
+ASSERT_TRUE(Pre->Preamble);
 EXPECT_EQ(Pre->Preamble->Version, "A");
 EXPECT_THAT(includes(Pre->Preamble),
 ElementsAre(""));
@@ -292,11 +293,13 @@
 S.runWithPreamble("ConsistentRead", Path, TUScheduler::Consistent,
   [&](Expected Pre) {
 ASSERT_TRUE(bool(Pre));
+ASSERT_TRUE(Pre->Preamble);
 EXPECT_EQ(Pre->Preamble->Version, "B");
 EXPECT_THAT(includes(Pre->Preamble),
 ElementsAre(""));
 ++CallbackCount;
   });
+S.blockUntilIdle(timeoutSeconds(10));
   }
   EXPECT_EQ(2, CallbackCount);
 }
@@ -853,15 +856,19 @@
   TUState(PreambleAction::Idle, ASTAction::RunningAction),
   // We build the preamble
   TUState(PreambleAction::Building, ASTAction::RunningAction),
-  // Preamble worker goes idle
+  // We built the preamble, and issued ast built on ASTWorker
+  // thread. Preambleworker goes idle afterwards.
   TUState(PreambleAction::Idle, ASTAction::RunningAction),
-  // We start building the ast
+  // Start task for building the ast, as a result of building
+  // preamble, on astworker thread.
+  TUState(PreambleAction::Idle, ASTAction::RunningAction),
+  // AST build starts.
   TUState(PreambleAction::Idle, ASTAction::Building),
-  // Built finished succesffully
+  // AST built finished successfully
   TUState(PreambleAction::Idle, ASTAction::Building),
-  // Rnning go to def
+  // Running go to def
   TUState(PreambleAction::Idle, ASTAction::RunningAction),
-  // both workers go idle
+  // ASTWorker goes idle.
   TUState(PreambleAction::Idle, ASTAction::Idle)));
 }
 
Index: clang-tools-extra/clangd/unittests/FileIndexTests.cpp
===
--- clang-tools-extra/clangd/unittests/FileIndexTests.cpp
+++ clang-tools-extra/clangd/unittests/FileIndexTests.cpp
@@ -286,7 +286,7 @@
 
   FileIndex Index;
   bool IndexUpdated = false;
-  buildPreamble(FooCpp, *CI, /*OldPreamble=*/nullptr, PI,
+  buildPreamble(FooCpp, *CI, PI,
 /*StoreInMemory=*/true,
 [&](ASTContext &Ctx, std::shared_ptr PP,
 const CanonicalIncludes &CanonIncludes) {
@@ -424,7 +424,7 @@
 }
 
 TEST(FileIndexTest, MergeMainFileSymbols) {
-  const char* CommonHeader = "void foo();";
+  const char *CommonHeader = "void foo();";
   TestTU Header = TestTU::withCode(CommonHeader);
   TestTU Cpp = TestTU::withCode("void foo() {}");
   Cpp.Filename = "foo.cpp";
Index: clang-tools-extra/clangd/TUScheduler.cpp
===
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -6,16 +6,46 @@
 //
 //===--

[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

So I think we need a careful description of the new model somewhere. Not so 
much on specific changes/constraints parts of the code operate under, but what 
it's trying to do.

My best understanding is:

- In general, read requests are processed in order by the ASTWorker, and get 
exactly the version of the file when the request was received. That is, the 
ASTworker queue interleaves updates and reads in the same order as LSP.
- Preambles are built on the PreambleWorker for a certain version of the file, 
and may be compatible or incompatible with subsequent versions. If a read is 
"picky" it may block the ASTWorker queue waiting for a compatible preamble, 
otherwise it will "patch up" an incompatible one.
- Diagnostics and indexes are updated opportunistically using onMainAST when an 
AST is built, but only if the used preamble is compatible. Publishing 
diagnostics from patched preambles is not allowed, but we also do not block 
waiting for an up-to-date preamble in order to generate publishable 
diagnostics. (Exception: if WantDiagnostics=Yes, we block).
- To ensure forward progress with diagnostics if the queue is full, when 
preamble is built we immediately build a "golden" AST from that version and 
publish diagnostics. To ensure diagnostics do not "go backwards", opportunistic 
diagnostics are suppressed if they don't use the latest preamble (the one with 
the last golden AST). This means the sequence of ASTs producing diagnostics is 
neatly partitioned into "epochs" of the preambles, the first in the epoch is 
always the golden version. (Exception: if WantDiagnostics=no, diagnostics are 
not emitted for the golden AST). Because these versions are built out-of-order 
(not interleaved with reads per LSP), the golden AST is not cached and reused 
for reads.

Is this about right?

Based on this, I do think the model would be much easier to reason about and 
verify if the golden AST was built on the ASTWorker thread in a queued task, 
rather than on the PreambleWorker thread in a callback. This means we 
understand allowed sequencing by looking at the queue rather than reasoning 
about mutexes/threads with multiple possible interleavings. By pushing the 
golden AST task to the front of the queue, it's more explicit that this is a 
priorities/scheduling decision. Following what happened in logs is probably 
easier due to less interleaving.
 (The callback could enqueue the task, or the PreambleWorker could just enqueue 
the task directly at that point - not sure the callback indirection buys 
anything).

One thing that seems complicated in the model is that the AST build needs to 
decide whether to block on a compatible preamble, but it's the following read 
that determines whether it needs to. In the worst case, the picky read hasn't 
even been scheduled yet. I think picky reads probably need to block on the 
target preamble and build their AST themselves if the cached one isn't 
suitable. If they "miss" their preamble (i.e. preambleVersion >= readVersion 
but the preamble isn't compatible) then I think we should fail the request 
(i.e. picky requests may be invalidated by subsequent preamble edits).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76725/new/

https://reviews.llvm.org/D76725



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay, 
javed.absar, ilya-biryukov.
Herald added a project: clang.
kadircet added a parent revision: D76304: [clangd] Update TUStatus api to 
accommodate preamble thread.

This is another step for out-of-order preamble builds. To keep the
diagnostic behavior same, we only build ASTs either with "reusable" preambles,
the ones that are fully applicable to a given ParseInput, or after building a
new preamble. Which is the same behaviour as what we do today.

ASTs built through preamble callbacks are not cached as they are built on a
different thread and ASTWorker heavily relies on being the only thread updating
cached ASTs. This results in possibly building some ASTs twice (when there's an
immediate read after a preamble built without any write in between).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76725

Files:
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp

Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -66,8 +66,7 @@
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
   auto Preamble =
-  buildPreamble(FullFilename, *CI,
-/*OldPreamble=*/nullptr, Inputs,
+  buildPreamble(FullFilename, *CI, Inputs,
 /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
   auto AST =
   buildAST(FullFilename, std::move(CI), Diags.take(), Inputs, Preamble);
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -570,6 +570,17 @@
   auto Bar = testPath("bar.cpp");
   auto Baz = testPath("baz.cpp");
 
+  // Build all of the files once, so that we've got their preambles ready.
+  updateWithCallback(S, Foo, SourceContents, WantDiagnostics::Yes,
+ [&BuiltASTCounter]() { ++BuiltASTCounter; });
+  updateWithCallback(S, Bar, SourceContents, WantDiagnostics::Yes,
+ [&BuiltASTCounter]() { ++BuiltASTCounter; });
+  updateWithCallback(S, Baz, SourceContents, WantDiagnostics::Yes,
+ [&BuiltASTCounter]() { ++BuiltASTCounter; });
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+  ASSERT_EQ(BuiltASTCounter.load(), 3);
+  BuiltASTCounter = 0;
+
   // Build one file in advance. We will not access it later, so it will be the
   // one that the cache will evict.
   updateWithCallback(S, Foo, SourceContents, WantDiagnostics::Yes,
@@ -697,26 +708,29 @@
   };
 
   // Test that subsequent updates with the same inputs do not cause rebuilds.
-  ASSERT_TRUE(DoUpdate(SourceContents));
-  ASSERT_FALSE(DoUpdate(SourceContents));
+  ASSERT_TRUE(DoUpdate(SourceContents));  // Builds preamble
+  ASSERT_TRUE(DoUpdate(SourceContents));  // Builds AST
+  ASSERT_FALSE(DoUpdate(SourceContents)); // Reuses it
 
   // Update to a header should cause a rebuild, though.
   Timestamps[Header] = time_t(1);
-  ASSERT_TRUE(DoUpdate(SourceContents));
-  ASSERT_FALSE(DoUpdate(SourceContents));
+  ASSERT_TRUE(DoUpdate(SourceContents));  // Builds preamble
+  ASSERT_TRUE(DoUpdate(SourceContents));  // Builds AST
+  ASSERT_FALSE(DoUpdate(SourceContents)); // Reuses AST
 
   // Update to the contents should cause a rebuild.
   auto OtherSourceContents = R"cpp(
   #include "foo.h"
   int c = d;
 )cpp";
-  ASSERT_TRUE(DoUpdate(OtherSourceContents));
-  ASSERT_FALSE(DoUpdate(OtherSourceContents));
+  ASSERT_TRUE(DoUpdate(OtherSourceContents));  // Builds AST, reusing preamble
+  ASSERT_FALSE(DoUpdate(OtherSourceContents)); // Reuses AST
 
   // Update to the compile commands should also cause a rebuild.
   CDB.ExtraClangFlags.push_back("-DSOMETHING");
-  ASSERT_TRUE(DoUpdate(OtherSourceContents));
-  ASSERT_FALSE(DoUpdate(OtherSourceContents));
+  ASSERT_TRUE(DoUpdate(SourceContents));   // Builds preamble
+  ASSERT_TRUE(DoUpdate(OtherSourceContents));  // Builds AST
+  ASSERT_FALSE(DoUpdate(OtherSourceContents)); // Reuses it
 }
 
 TEST_F(TUSchedulerTests, ForceRebuild) {
@@ -732,6 +746,10 @@
 
   ParseInputs Inputs = getInputs(Source, SourceContents);
 
+  // Send an initial update to make sure we've got preamble ready.
+  S.update(Source, Inputs, WantDiagnostics::Yes);
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+
   // Update the source contents, which should trigger an initial build with
   // the header file