[clangd] Avoid emitting Queued status when we are able to acquire the Barrier.

[ilya-biryukov]

clangd/TUScheduler.cpp

652

Maybe simplify the code a bit?

// Replacing these two lines:
//  emitTUStatus({TUAction::Queued, Req.Name});
//  std::lock_guard<Semaphore> BarrierLock(Barrier);

// With:
std::unique_lock<Semaphore> BarrierLock(Barrier, std::try_to_lock);
if (!Lock.owns_lock()) {
  emitTUStatus({TUAction::Queued, Req.Name});
  Lock.lock();
}
// Rest of the code is the same...

Avoids creating two locks and does not require helper lamdbas.

[hokein]

Address review comment.

clangd/TUScheduler.cpp

652

ah, good point.

[ilya-biryukov]

LGTM

clangd/TUScheduler.cpp

Request Req;
 
         wait(Lock, RequestsCV, Wait);
       }
       Req = std::move(Requests.front());
       // Leave it on the queue for now, so waiters don't see an empty queue.
     } // unlock Mutex
 
     {
-      // FIXME: only emit this status when the Barrier couldn't be acquired.
+      std::unique_lock<Semaphore> Lock(Barrier, std::try_to_lock);
+      if (!Lock.owns_lock()) {
         emitTUStatus({TUAction::Queued, Req.Name});
-      std::lock_guard<Semaphore> BarrierLock(Barrier);
+        Lock.lock();
+      }
       WithContext Guard(std::move(Req.Ctx));
       trace::Span Tracer(Req.Name);
       emitTUStatus({TUAction::RunningAction, Req.Name});
       Req.Action();
     }
 
     bool IsEmpty = false;
     {
       std::lock_guard<std::mutex> Lock(Mutex);
       Requests.pop_front();
       IsEmpty = Requests.empty();
     }
     if (IsEmpty)
       emitTUStatus({TUAction::Idle, /*Name*/ ""});
     RequestsCV.notify_all();
   }
 }
 
 Deadline ASTWorker::scheduleLocked() {

clangd/Threading.h

private:
   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();
 
 private:
   std::mutex Mutex;
   std::condition_variable SlotsChanged;
   std::size_t FreeSlots;
 };

clangd/Threading.cpp

 
 void Notification::wait() const {
   std::unique_lock<std::mutex> Lock(Mu);
   CV.wait(Lock, [this] { 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;
+    return true;
+  }
+  return false;
+}
+
 void Semaphore::lock() {
   trace::Span Span("WaitForFreeSemaphoreSlot");
   // trace::Span can also acquire locks in ctor and dtor, we make sure it
   // happens when Semaphore's own lock is not held.
   {
     std::unique_lock<std::mutex> Lock(Mutex);
     SlotsChanged.wait(Lock, [&]() { return FreeSlots > 0; });
     --FreeSlots;

unittests/clangd/TUSchedulerTests.cpp

Server.findDefinitions(testPath("foo.cpp"), Code.point(),
                            ASSERT_TRUE((bool)Result);
                          });
 
   ASSERT_TRUE(Server.blockUntilIdleForTest());
 
   EXPECT_THAT(CaptureTUStatus.AllStatus,
               ElementsAre(
                   // Statuses of "Update" action.
-                  TUState(TUAction::Queued, "Update"),
                   TUState(TUAction::RunningAction, "Update"),
                   TUState(TUAction::BuildingPreamble, "Update"),
                   TUState(TUAction::BuildingFile, "Update"),
 
                   // Statuses of "Definitions" action
-                  TUState(TUAction::Queued, "Definitions"),
                   TUState(TUAction::RunningAction, "Definitions"),
                   TUState(TUAction::Idle, /*No action*/ "")));
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang