[clangd] Speed up a slow sleeping testcase.

[sammccall]

Summary

This testcase runs slowly due to 3.2s of sleeps = 2 + 1 + 0.2s.
After this patch it has 0.55s only.

Reduced by:

• observed that the last test was bogus: we were sleeping until the queue was idle, effectively just a second copy of the first test. This avoids 1s sleep.
• when waiting for debounce, sleep only until test passes, not for enough time to be safe (in practice was 2x debounce time, now 1x debounce time)
• scaling delays down by a factor of 2 (note: factor of 10 caused bot failures)

[ilya-biryukov]

Many thanks for improving the running time, this slow test has bugged me since the day I first ran it.
I'd even be happy to remove it completely. It's slow, possibly flaky, so IMO the corresponding check does not carry its weight.

I'm not sure if we introduced a new race condition there that will cause flaky test, see the comment.
Otherwise LGTM.

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

272

This is potentially a race in the test that we did not have before, right?
There is probably much less work in the main test thread, but we probably can still see the failure if we are unlucky with how the OS schedules us.

My stance is it's better to have no tests for certain behaviours than flaky tests.
But if you find it useful let's try to run it and see whether my suspicions are unfounded, don't want to block the change on it.

[sammccall]

In D125103#3497140, @ilya-biryukov wrote:

Many thanks for improving the running time, this slow test has bugged me since the day I first ran it.
I'd even be happy to remove it completely. It's slow, possibly flaky, so IMO the corresponding check does not carry its weight.

I agree in general, unfortunately this test is really valuable:

• it tests something critical
• it tests something tricky
• it's the only test of that functionality
• it's prevented real bugs in the past
• I can't see how to replace it with reasonable effort
• it doesn't seem to be flaky at all in practice => hard to justify an unreasonable effort

So I want to keep it, and I promise to feel bad about it.
(I think this is the only ridiculous sleeping test we have in clangd...)

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

272

Yes-ish - it's the same thing as the race on the first updateWithDiags. In both cases we're hoping that the next event happens within 450/500ms so the debounce doesn't expire.

AFAIK this hasn't been flaky at all in practice (albeit at 1000ms), so until we have a better test (see above) I'd like to keep it :-(

clang-tools-extra/clangd/support/Threading.h

 #include <memory>
 #include <mutex>
 #include <thread>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 
-/// A threadsafe flag that is initially clear.
-class Notification {
-public:
-  // Sets the flag. No-op if already set.
-  void notify();
-  // Blocks until flag is set.
-  void wait() const;
-
-private:
-  bool Notified = false;
-  mutable std::condition_variable CV;
-  mutable std::mutex Mu;
-};
-
 /// Limits the number of threads that can acquire the lock at the same time.
 class Semaphore {
 public:
   Semaphore(std::size_t MaxLocks);
 
   bool try_lock();
   void lock();
   void unlock();
LLVM_NODISCARD bool wait(std::unique_lock<std::mutex> &Lock,
   while (!F()) {
     if (D.expired())
       return false;
     wait(Lock, CV, D);
   }
   return true;
 }
 
+/// A threadsafe flag that is initially clear.
+class Notification {
+public:
+  // Sets the flag. No-op if already set.
+  void notify();
+  // Blocks until flag is set.
+  void wait() const { (void)wait(Deadline::infinity()); }
+  LLVM_NODISCARD bool wait(Deadline D) const;
+
+private:
+  bool Notified = false;
+  mutable std::condition_variable CV;
+  mutable std::mutex Mu;
+};
+
 /// Runs tasks on separate (detached) threads and wait for all tasks to finish.
 /// Objects that need to spawn threads can own an AsyncTaskRunner to ensure they
 /// all complete on destruction.
 class AsyncTaskRunner {
 public:
   /// Destructor waits for all pending tasks to finish.
   ~AsyncTaskRunner();
 

clang-tools-extra/clangd/support/Threading.cpp

void Notification::notify() {
     std::lock_guard<std::mutex> Lock(Mu);
     Notified = true;
     // Broadcast with the lock held. This ensures that it's safe to destroy
     // a Notification after wait() returns, even from another thread.
     CV.notify_all();
   }
 }
 
-void Notification::wait() const {
+bool Notification::wait(Deadline D) const {
   std::unique_lock<std::mutex> Lock(Mu);
-  CV.wait(Lock, [this] { return Notified; });
+  return clangd::wait(Lock, CV, D, [&] { return Notified; });
 }
 
 Semaphore::Semaphore(std::size_t MaxLocks) : FreeSlots(MaxLocks) {}
 
 bool Semaphore::try_lock() {
   std::unique_lock<std::mutex> Lock(Mutex);
   if (FreeSlots > 0) {
     --FreeSlots;

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

std::atomic<int> CallbackCount(0);
     Ready.notify();
 
     ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
   }
   EXPECT_EQ(2, CallbackCount);
 }
 
 TEST_F(TUSchedulerTests, Debounce) {
-  std::atomic<int> CallbackCount(0);
-  {
-    auto Opts = optsForTest();
-    Opts.UpdateDebounce = DebouncePolicy::fixed(std::chrono::seconds(1));
-    TUScheduler S(CDB, Opts, captureDiags());
-    // FIXME: we could probably use timeouts lower than 1 second here.
-    auto Path = testPath("foo.cpp");
-    updateWithDiags(S, Path, "auto (debounced)", WantDiagnostics::Auto,
-                    [&](std::vector<Diag>) {
-                      ADD_FAILURE()
-                          << "auto should have been debounced and canceled";
-                    });
-    std::this_thread::sleep_for(std::chrono::milliseconds(200));
-    updateWithDiags(S, Path, "auto (timed out)", WantDiagnostics::Auto,
-                    [&](std::vector<Diag>) { ++CallbackCount; });
-    std::this_thread::sleep_for(std::chrono::seconds(2));
-    updateWithDiags(S, Path, "auto (shut down)", WantDiagnostics::Auto,
-                    [&](std::vector<Diag>) { ++CallbackCount; });
+  Opts.UpdateDebounce = DebouncePolicy::fixed(std::chrono::milliseconds(500));
+  // Issue a write that's going to be debounced away.
+  // Sleep a bit to verify that it's really debounce that's holding diagnostics.
+  std::this_thread::sleep_for(std::chrono::milliseconds(50));
+
+  // Issue another write, this time we'll wait for its diagnostics.
+  Notification N;
+                  [&](std::vector<Diag>) { N.notify(); });
+  EXPECT_TRUE(N.wait(timeoutSeconds(1)));
 
-    ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
-  }
-  EXPECT_EQ(2, CallbackCount);
+  // Once we start shutting down the TUScheduler, this one becomes a dead write.
+  updateWithDiags(S, Path, "auto (discarded)", WantDiagnostics::Auto,
+                  [&](std::vector<Diag>) {
+                    ADD_FAILURE()
+                        << "auto should have been discarded (dead write)";
+                  });
 }
 
 TEST_F(TUSchedulerTests, Cancellation) {
   // We have the following update/read sequence
   //   U0
   //   U1(WantDiags=Yes) <-- cancelled
   //    R1               <-- cancelled
   //   U2(WantDiags=Yes) <-- cancelled