[clangd] Run SignatureHelp using an up-to-date preamble, waiting if needed.

[sammccall]

Summary

After code completion inserts a header, running signature help using the old
preamble will usually fail. So we add support for consistent preamble reads.

[sammccall]

(This needs new TUScheduler tests for Consistent mode, but would like some feedback on the implementation first)

[sammccall]

Finish a sentence

[ilya-biryukov]

The implementation is really simple. LG!
To get proper operation order in tests, we can wait in the diagnostics callback that runs on the worker thread (IIRC, we do that in some of the other tests too).

clangd/TUScheduler.cpp

188 ↗

(On Diff #163121)

As discussed offline, it looks like it shouldn't be a problem to call it from the working thread, so we could probably remove this restriction from the comment.
Not that it's the right thing to do or that we have a use-case for that yet.

474 ↗

(On Diff #163121)

It seems we could remove the special-casing of RunSync and still get correct results (the Requests queue is always empty).
But feel free to keep it for clarity.

clangd/TUScheduler.h

123 ↗

(On Diff #163121)

Maybe use a strongly-typed enum outside the class?
TUScheduler::Stale will turn into PreambleConsistency::Stale at the call-site. The main upside is that it does not pollute completions inside the class with enumerators.

Just a suggestion, feel free to ignore.

[sammccall]

Add test, address comments.

[ilya-biryukov]

LGTM

[sammccall]

clangd/TUScheduler.cpp

474 ↗

(On Diff #163121)

Right, of course :-)
Replaced this with an assert before we write to the queue.

clangd/TUScheduler.h

123 ↗

(On Diff #163121)

Yeah, the downside to that is that it *does* pollute the clangd:: namespace. So both are a bit sad.

This header is fairly widely visible (since it's included by clangdserver) and this API is fairly narrowly interesting, so as far as completion goes I think I prefer it being hidden in TUScheduler.

[yroux]

Hi Sam,

It took a very long time to identify it, but this commit broke ARMv7
bots, where this test hangs. Logs are available here (initial ones
are too old):

http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/3685/steps/ninja%20check%201/logs/stdiohttp://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/3685/steps/ninja%20check%201/logs/stdio

Thanks,
YvanOn Thu, 30 Aug 2018 at 17:15, Sam McCall via Phabricator via
cfe-commits <[e-mail]> wrote:

sammccall added inline comments.

Comment at: clangd/TUScheduler.cpp:474
+ llvm::unique_function<void(std::shared_ptr<const PreambleData>)> Callback) {
+ if (RunSync)

+ return Callback(getPossiblyStalePreamble());

ilya-biryukov wrote:

It seems we could remove the special-casing of RunSync and still get correct results (the Requests queue is always empty).
But feel free to keep it for clarity.

Right, of course :-)
Replaced this with an assert before we write to the queue.

Comment at: clangd/TUScheduler.h:123
+ /// Controls whether preamble reads wait for the preamble to be up-to-date.
+ enum PreambleConsistency {

+ /// The preamble is generated from the current version of the file.

ilya-biryukov wrote:

Maybe use a strongly-typed enum outside the class?
TUScheduler::Stale will turn into PreambleConsistency::Stale at the call-site. The main upside is that it does not pollute completions inside the class with enumerators.

Just a suggestion, feel free to ignore.

Yeah, the downside to that is that it *does* pollute the clangd:: namespace. So both are a bit sad.

This header is fairly widely visible (since it's included by clangdserver) and this API is fairly narrowly interesting, so as far as completion goes I think I prefer it being hidden in TUScheduler.

Repository:

rL LLVM

https://reviews.llvm.org/D51438

---

cfe-commits mailing list
[e-mail]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[yroux]

Seems that it was fixed earlier today.

Thanks,
Yvan

clang-tools-extra/trunk/clangd/ClangdServer.cpp

if (SpecFuzzyFind && SpecFuzzyFind->NewReq.hasValue()) {
           SpecFuzzyFind->NewReq.getValue();
     }
     // SpecFuzzyFind is only destroyed after speculative fuzzy find finishes.
     // We don't want `codeComplete` to wait for the async call if it doesn't use
     // the result (e.g. non-index completion, speculation fails), so that `CB`
     // is called as soon as results are available.
   };
 
-  WorkScheduler.runWithPreamble("CodeComplete", File,
+  // We use a potentially-stale preamble because latency is critical here.
+  WorkScheduler.runWithPreamble("CodeComplete", File, TUScheduler::Stale,
                                 Bind(Task, File.str(), std::move(CB)));
   return TH;
 }
 
 void ClangdServer::signatureHelp(PathRef File, Position Pos,
                                  Callback<SignatureHelp> CB) {
 
   auto PCHs = this->PCHs;
   auto FS = FSProvider.getFileSystem();
   auto *Index = this->Index;
   auto Action = [Pos, FS, PCHs, Index](Path File, Callback<SignatureHelp> CB,
                                        llvm::Expected<InputsAndPreamble> IP) {
     if (!IP)
       return CB(IP.takeError());
 
     auto PreambleData = IP->Preamble;
     CB(clangd::signatureHelp(File, IP->Command,
                              PreambleData ? &PreambleData->Preamble : nullptr,
                              IP->Contents, Pos, FS, PCHs, Index));
   };
 
-  WorkScheduler.runWithPreamble("SignatureHelp", File,
+  // Unlike code completion, we wait for an up-to-date preamble here.
+  // Signature help is often triggered after code completion. If the code
+  // completion inserted a header to make the symbol available, then using
+  // the old preamble would yield useless results.
+  WorkScheduler.runWithPreamble("SignatureHelp", File, TUScheduler::Consistent,
                                 Bind(Action, File.str(), std::move(CB)));
 }
 
 llvm::Expected<tooling::Replacements>
 ClangdServer::formatRange(StringRef Code, PathRef File, Range Rng) {
   llvm::Expected<size_t> Begin = positionToOffset(Code, Rng.start);
   if (!Begin)
     return Begin.takeError();

clang-tools-extra/trunk/clangd/TUScheduler.h

public:
   /// ready. The AST passed to \p Action refers to the version of \p File
   /// tracked at the time of the call, even if new updates are received before
   /// \p Action is executed.
   /// If an error occurs during processing, it is forwarded to the \p Action
   /// callback.
   void runWithAST(llvm::StringRef Name, PathRef File,
                   Callback<InputsAndAST> Action);
 
-  /// Schedule an async read of the Preamble.
-  /// The preamble may be stale, generated from an older version of the file.
-  /// Reading from locations in the preamble may cause the files to be re-read.
+  /// Controls whether preamble reads wait for the preamble to be up-to-date.
+  enum PreambleConsistency {
+    /// The preamble is generated from the current version of the file.
+    /// If the content was recently updated, we will wait until we have a
+    /// preamble that reflects that update.
+    /// This is the slowest option, and may be delayed by other tasks.
+    Consistent,
+    /// The preamble may be generated from an older version of the file.
+    /// Reading from locations in the preamble may cause files to be re-read.
     /// This gives callers two options:
-  /// - validate that the preamble is still valid, and only use it in this case
-  /// - accept that preamble contents may be outdated, and try to avoid reading
-  ///   source code from headers.
+    /// - validate that the preamble is still valid, and only use it if so
+    /// - accept that the preamble contents may be outdated, and try to avoid
+    ///   reading source code from headers.
+    /// This is the fastest option, usually a preamble is available immediately.
+    Stale,
+  };
+  /// Schedule an async read of the preamble.
   /// If there's no preamble yet (because the file was just opened), we'll wait
-  /// for it to build. The preamble may still be null if it fails to build or is
-  /// empty.
-  /// If an error occurs during processing, it is forwarded to the \p Action
-  /// callback.
+  /// for it to build. The result may be null if it fails to build or is empty.
+  /// If an error occurs, it is forwarded to the \p Action callback.
   void runWithPreamble(llvm::StringRef Name, PathRef File,
+                       PreambleConsistency Consistency,
                        Callback<InputsAndPreamble> Action);
 
   /// Wait until there are no scheduled or running tasks.
   /// Mostly useful for synchronizing tests.
   bool blockUntilIdle(Deadline D) const;
 
 private:
   /// This class stores per-file data in the Files map.

clang-tools-extra/trunk/clangd/TUScheduler.cpp

public:
   void update(ParseInputs Inputs, WantDiagnostics,
               llvm::unique_function<void(std::vector<Diag>)> OnUpdated);
   void
   runWithAST(llvm::StringRef Name,
              llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action);
   bool blockUntilIdle(Deadline Timeout) const;
 
   std::shared_ptr<const PreambleData> getPossiblyStalePreamble() const;
+  /// Obtain a preamble reflecting all updates so far. Threadsafe.
+  /// It may be delivered immediately, or later on the worker thread.
+  void getCurrentPreamble(
+      llvm::unique_function<void(std::shared_ptr<const PreambleData>)>);
   /// Wait for the first build of preamble to finish. Preamble itself can be
   /// accessed via getPossibleStalePreamble(). Note that this function will
   /// return after an unsuccessful build of the preamble too, i.e. result of
   /// getPossiblyStalePreamble() can be null even after this function returns.
   void waitForFirstPreamble() const;
 
   std::size_t getUsedBytes() const;
   bool isASTCached() const;

 }
 
 std::shared_ptr<const PreambleData>
 ASTWorker::getPossiblyStalePreamble() const {
   std::lock_guard<std::mutex> Lock(Mutex);
   return LastBuiltPreamble;
 }
 
+void ASTWorker::getCurrentPreamble(
+    llvm::unique_function<void(std::shared_ptr<const PreambleData>)> Callback) {
+  // We could just call startTask() to throw the read on the queue, knowing
+  // it will run after any updates. But we know this task is cheap, so to
+  // improve latency we cheat: insert it on the queue after the last update.
+  std::unique_lock<std::mutex> Lock(Mutex);
+  auto LastUpdate =
+      std::find_if(Requests.rbegin(), Requests.rend(),
+                   [](const Request &R) { return R.UpdateType.hasValue(); });
+  // If there were no writes in the queue, the preamble is ready now.
+  if (LastUpdate == Requests.rend()) {
+    Lock.unlock();
+    return Callback(getPossiblyStalePreamble());
+  }
+  assert(!RunSync && "Running synchronously, but queue is non-empty!");
+  Requests.insert(LastUpdate.base(),
+                  Request{Bind(
+                              [this](decltype(Callback) Callback) {
+                                Callback(getPossiblyStalePreamble());
+                              },
+                              std::move(Callback)),
+                          "GetPreamble", steady_clock::now(),
+                          Context::current().clone(),
+                          /*UpdateType=*/llvm::None});
+  Lock.unlock();
+  RequestsCV.notify_all();
+}
+
 void ASTWorker::waitForFirstPreamble() const {
   PreambleWasBuilt.wait();
 }
 
 std::size_t ASTWorker::getUsedBytes() const {
   // Note that we don't report the size of ASTs currently used for processing
   // the in-flight requests. We used this information for debugging purposes
   // only, so this should be fine.
Action(llvm::make_error<llvm::StringError>(
         llvm::errc::invalid_argument));
     return;
   }
 
   It->second->Worker->runWithAST(Name, std::move(Action));
 }
 
 void TUScheduler::runWithPreamble(
-    llvm::StringRef Name, PathRef File,
+    llvm::StringRef Name, PathRef File, PreambleConsistency Consistency,
     llvm::unique_function<void(llvm::Expected<InputsAndPreamble>)> Action) {
   auto It = Files.find(File);
   if (It == Files.end()) {
     Action(llvm::make_error<llvm::StringError>(
         "trying to get preamble for non-added document",
         llvm::errc::invalid_argument));
     return;
   }
 
   if (!PreambleTasks) {
     trace::Span Tracer(Name);
     SPAN_ATTACH(Tracer, "file", File);
     std::shared_ptr<const PreambleData> Preamble =
         It->second->Worker->getPossiblyStalePreamble();
     Action(InputsAndPreamble{It->second->Contents, It->second->Command,
                              Preamble.get()});
     return;
   }
 
+  // Future is populated if the task needs a specific preamble.
+  std::future<std::shared_ptr<const PreambleData>> ConsistentPreamble;
+  if (Consistency == Consistent) {
+    std::promise<std::shared_ptr<const PreambleData>> Promise;
+    ConsistentPreamble = Promise.get_future();
+    It->second->Worker->getCurrentPreamble(Bind(
+        [](decltype(Promise) Promise,
+           std::shared_ptr<const PreambleData> Preamble) {
+          Promise.set_value(std::move(Preamble));
+        },
+        std::move(Promise)));
+  }
+
   std::shared_ptr<const ASTWorker> Worker = It->second->Worker.lock();
   auto Task = [Worker, this](std::string Name, std::string File,
                              std::string Contents,
                              tooling::CompileCommand Command, Context Ctx,
+                             decltype(ConsistentPreamble) ConsistentPreamble,
                              decltype(Action) Action) mutable {
+    std::shared_ptr<const PreambleData> Preamble;
+    if (ConsistentPreamble.valid()) {
+      Preamble = ConsistentPreamble.get();
+    } else {
       // We don't want to be running preamble actions before the preamble was
       // built for the first time. This avoids extra work of processing the
       // preamble headers in parallel multiple times.
       Worker->waitForFirstPreamble();
+      Preamble = Worker->getPossiblyStalePreamble();
+    }
 
     std::lock_guard<Semaphore> BarrierLock(Barrier);
     WithContext Guard(std::move(Ctx));
     trace::Span Tracer(Name);
     SPAN_ATTACH(Tracer, "file", File);
-    std::shared_ptr<const PreambleData> Preamble =
-        Worker->getPossiblyStalePreamble();
     Action(InputsAndPreamble{Contents, Command, Preamble.get()});
   };
 
   PreambleTasks->runAsync(
       "task:" + llvm::sys::path::filename(File),
       Bind(Task, std::string(Name), std::string(File), It->second->Contents,
            It->second->Command,
            Context::current().derive(kFileBeingProcessed, File),
-           std::move(Action)));
+           std::move(ConsistentPreamble), std::move(Action)));
 }
 
 std::vector<std::pair<Path, std::size_t>>
 TUScheduler::getUsedBytesPerFile() const {
   std::vector<std::pair<Path, std::size_t>> Result;
   Result.reserve(Files.size());
   for (auto &&PathAndFile : Files)
     Result.push_back(

clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp

 
 namespace clang {
 namespace clangd {
 namespace {
 
 using ::testing::_;
 using ::testing::AnyOf;
 using ::testing::Each;
+using ::testing::ElementsAre;
 using ::testing::Pair;
 using ::testing::Pointee;
 using ::testing::UnorderedElementsAre;
 
 void ignoreUpdate(llvm::Optional<std::vector<Diag>>) {}
 void ignoreError(llvm::Error Err) {
   handleAllErrors(std::move(Err), [](const llvm::ErrorInfoBase &) {});
 }
TEST_F(TUSchedulerTests, MissingFiles) {
   S.update(Added, getInputs(Added, ""), WantDiagnostics::No, ignoreUpdate);
 
   // Assert each operation for missing file is an error (even if it's available
   // in VFS).
   S.runWithAST("", Missing, [&](llvm::Expected<InputsAndAST> AST) {
     ASSERT_FALSE(bool(AST));
     ignoreError(AST.takeError());
   });
-  S.runWithPreamble("", Missing,
+  S.runWithPreamble("", Missing, TUScheduler::Stale,
                     [&](llvm::Expected<InputsAndPreamble> Preamble) {
                       ASSERT_FALSE(bool(Preamble));
                       ignoreError(Preamble.takeError());
                     });
   // remove() shouldn't crash on missing files.
   S.remove(Missing);
 
   // Assert there aren't any errors for added file.
   S.runWithAST("", Added, [&](llvm::Expected<InputsAndAST> AST) {
     EXPECT_TRUE(bool(AST));
   });
-  S.runWithPreamble("", Added, [&](llvm::Expected<InputsAndPreamble> Preamble) {
+  S.runWithPreamble("", Added, TUScheduler::Stale,
+                    [&](llvm::Expected<InputsAndPreamble> Preamble) {
                       EXPECT_TRUE(bool(Preamble));
                     });
   S.remove(Added);
 
   // Assert that all operations fail after removing the file.
   S.runWithAST("", Added, [&](llvm::Expected<InputsAndAST> AST) {
     ASSERT_FALSE(bool(AST));
     ignoreError(AST.takeError());
   });
-  S.runWithPreamble("", Added, [&](llvm::Expected<InputsAndPreamble> Preamble) {
+  S.runWithPreamble("", Added, TUScheduler::Stale,
+                    [&](llvm::Expected<InputsAndPreamble> Preamble) {
                       ASSERT_FALSE(bool(Preamble));
                       ignoreError(Preamble.takeError());
                     });
   // remove() shouldn't crash on missing files.
   S.remove(Added);
 }
 
 TEST_F(TUSchedulerTests, WantDiagnostics) {
   std::atomic<int> CallbackCount(0);
   {
     // To avoid a racy test, don't allow tasks to actualy run on the worker
S.update(Path, getInputs(Path, "auto (timed out)"), WantDiagnostics::Auto,
              [&](std::vector<Diag> Diags) { ++CallbackCount; });
     std::this_thread::sleep_for(std::chrono::seconds(2));
     S.update(Path, getInputs(Path, "auto (shut down)"), WantDiagnostics::Auto,
              [&](std::vector<Diag> Diags) { ++CallbackCount; });
   }
   EXPECT_EQ(2, CallbackCount);
 }
 
+static std::vector<std::string> includes(const PreambleData *Preamble) {
+  std::vector<std::string> Result;
+  if (Preamble)
+    for (const auto &Inclusion : Preamble->Includes.MainFileIncludes)
+      Result.push_back(Inclusion.Written);
+  return Result;
+}
+
+TEST_F(TUSchedulerTests, PreambleConsistency) {
+  std::atomic<int> CallbackCount(0);
+  {
+    Notification InconsistentReadDone; // Must live longest.
+    TUScheduler S(
+        getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+        noopParsingCallbacks(),
+        /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+        ASTRetentionPolicy());
+    auto Path = testPath("foo.cpp");
+    // Schedule two updates (A, B) and two preamble reads (stale, consistent).
+    // The stale read should see A, and the consistent read should see B.
+    // (We recognize the preambles by their included files).
+    S.update(Path, getInputs(Path, "#include <A>"), WantDiagnostics::Yes,
+             [&](std::vector<Diag> Diags) {
+               // This callback runs in between the two preamble updates.
+
+               // This blocks update B, preventing it from winning the race
+               // against the stale read.
+               // If the first read was instead consistent, this would deadlock.
+               InconsistentReadDone.wait();
+               // This delays update B, preventing it from winning a race
+               // against the consistent read. The consistent read sees B
+               // only because it waits for it.
+               // If the second read was stale, it would usually see A.
+               std::this_thread::sleep_for(std::chrono::milliseconds(100));
+             });
+    S.update(Path, getInputs(Path, "#include <B>"), WantDiagnostics::Yes,
+             [&](std::vector<Diag> Diags) {});
+
+    S.runWithPreamble("StaleRead", Path, TUScheduler::Stale,
+                      [&](llvm::Expected<InputsAndPreamble> Pre) {
+                        ASSERT_TRUE(bool(Pre));
+                        assert(bool(Pre));
+                        EXPECT_THAT(includes(Pre->Preamble),
+                                    ElementsAre("<A>"));
+                        InconsistentReadDone.notify();
+                        ++CallbackCount;
+                      });
+    S.runWithPreamble("ConsistentRead", Path, TUScheduler::Consistent,
+                      [&](llvm::Expected<InputsAndPreamble> Pre) {
+                        ASSERT_TRUE(bool(Pre));
+                        EXPECT_THAT(includes(Pre->Preamble),
+                                    ElementsAre("<B>"));
+                        ++CallbackCount;
+                      });
+  }
+  EXPECT_EQ(2, CallbackCount);
+}
+
 TEST_F(TUSchedulerTests, ManyUpdates) {
   const int FilesCount = 3;
   const int UpdatesPerFile = 10;
 
   std::mutex Mut;
   int TotalASTReads = 0;
   int TotalPreambleReads = 0;
   int TotalUpdates = 0;
for (int FileI = 0; FileI < FilesCount; ++FileI) {
                              File,
                              *TUScheduler::getFileBeingProcessedInContext());
                        });
         }
 
         {
           WithContextValue WithNonce(NonceKey, ++Nonce);
           S.runWithPreamble(
-              "CheckPreamble", File,
+              "CheckPreamble", File, TUScheduler::Stale,
               [File, Inputs, Nonce, &Mut, &TotalPreambleReads](
                   llvm::Expected<InputsAndPreamble> Preamble) {
                 EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce));
 
                 ASSERT_TRUE((bool)Preamble);
                 EXPECT_EQ(Preamble->Contents, Inputs.Contents);
 
                 std::lock_guard<std::mutex> Lock(Mut);
TEST_F(TUSchedulerTests, EmptyPreamble) {
   Timestamps[Header] = time_t(0);
   auto WithPreamble = R"cpp(
     #include "foo.h"
     int main() {}
   )cpp";
   auto WithEmptyPreamble = R"cpp(int main() {})cpp";
   S.update(Foo, getInputs(Foo, WithPreamble), WantDiagnostics::Auto,
            [](std::vector<Diag>) {});
-  S.runWithPreamble("getNonEmptyPreamble", Foo,
+  S.runWithPreamble("getNonEmptyPreamble", Foo, TUScheduler::Stale,
                     [&](llvm::Expected<InputsAndPreamble> Preamble) {
                       // We expect to get a non-empty preamble.
                       EXPECT_GT(cantFail(std::move(Preamble))
                                     .Preamble->Preamble.getBounds()
                                     .Size,
                                 0u);
                     });
   // Wait for the preamble is being built.
   ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
 
   // Update the file which results in an empty preamble.
   S.update(Foo, getInputs(Foo, WithEmptyPreamble), WantDiagnostics::Auto,
            [](std::vector<Diag>) {});
   // Wait for the preamble is being built.
   ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
-  S.runWithPreamble("getEmptyPreamble", Foo,
+  S.runWithPreamble("getEmptyPreamble", Foo, TUScheduler::Stale,
                     [&](llvm::Expected<InputsAndPreamble> Preamble) {
                       // We expect to get an empty preamble.
                       EXPECT_EQ(cantFail(std::move(Preamble))
                                     .Preamble->Preamble.getBounds()
                                     .Size,
                                 0u);
                     });
 }
TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
   )cpp";
   constexpr int ReadsToSchedule = 10;
   std::mutex PreamblesMut;
   std::vector<const void *> Preambles(ReadsToSchedule, nullptr);
   S.update(Foo, getInputs(Foo, NonEmptyPreamble), WantDiagnostics::Auto,
            [](std::vector<Diag>) {});
   for (int I = 0; I < ReadsToSchedule; ++I) {
     S.runWithPreamble(
-        "test", Foo,
+        "test", Foo, TUScheduler::Stale,
         [I, &PreamblesMut, &Preambles](llvm::Expected<InputsAndPreamble> IP) {
           std::lock_guard<std::mutex> Lock(PreamblesMut);
           Preambles[I] = cantFail(std::move(IP)).Preamble;
         });
   }
   ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
   // Check all actions got the same non-null preamble.
   std::lock_guard<std::mutex> Lock(PreamblesMut);