[lldb] Implement coalescing of disjoint progress events (llvm#84854)

This implements coalescing of progress events using a timeout, as
discussed in the RFC on Discourse [1]. This PR consists of two commits
which, depending on the feedback, I may split up into two PRs. For now,
I think it's easier to review this as a whole.

1. The first commit introduces a new generic `Alarm` class. The class
lets you to schedule a function (callback) to be executed after a given
timeout expires. You can cancel and reset a callback before its
corresponding timeout expires. It achieves this with the help of a
worker thread that sleeps until the next timeout expires. The only
guarantee it provides is that your function is called no sooner than the
requested timeout. Because the callback is called directly from the
worker thread, a long running callback could potentially block the
worker thread. I intentionally kept the implementation as simple as
possible while addressing the needs for the `ProgressManager` use case.
If we want to rely on this somewhere else, we can reassess whether we
need to address those limitations.

2. The second commit uses the Alarm class to coalesce progress events.
To recap the Discourse discussion, when multiple progress events with
the same title execute in close succession, they get broadcast as one to
`eBroadcastBitProgressCategory`. The `ProgressManager` keeps track of
the in-flight progress events and when the refcount hits zero, the Alarm
class is used to schedule broadcasting the event. If a new progress
event comes in before the alarm fires, the alarm is reset (and the
process repeats when the new progress event ends). If no new event comes
in before the timeout expires, the progress event is broadcast.

[1]
https://discourse.llvm.org/t/rfc-improve-lldb-progress-reporting/75717/

Author: JDevlieghere

lldb/include/lldb/Core/Progress.h

@@ -9,6 +9,7 @@
 #ifndef LLDB_CORE_PROGRESS_H
 #define LLDB_CORE_PROGRESS_H
 
+#include "lldb/Host/Alarm.h"
 #include "lldb/lldb-forward.h"
 #include "lldb/lldb-types.h"
 #include "llvm/ADT/StringMap.h"
@@ -150,19 +151,45 @@ class ProgressManager {
   void Increment(const Progress::ProgressData &);
   void Decrement(const Progress::ProgressData &);
 
+  static void Initialize();
+  static void Terminate();
+  static bool Enabled();
   static ProgressManager &Instance();
 
-private:
+protected:
   enum class EventType {
     Begin,
     End,
   };
   static void ReportProgress(const Progress::ProgressData &progress_data,
                              EventType type);
 
-  llvm::StringMap<std::pair<uint64_t, Progress::ProgressData>>
-      m_progress_category_map;
-  std::mutex m_progress_map_mutex;
+  static std::optional<ProgressManager> &InstanceImpl();
+
+  /// Helper function for reporting progress when the alarm in the corresponding
+  /// entry in the map expires.
+  void Expire(llvm::StringRef key);
+
+  /// Entry used for bookkeeping.
+  struct Entry {
+    /// Reference count used for overlapping events.
+    uint64_t refcount = 0;
+
+    /// Data used to emit progress events.
+    Progress::ProgressData data;
+
+    /// Alarm handle used when the refcount reaches zero.
+    Alarm::Handle handle = Alarm::INVALID_HANDLE;
+  };
+
+  /// Map used for bookkeeping.
+  llvm::StringMap<Entry> m_entries;
+
+  /// Mutex to provide the map.
+  std::mutex m_entries_mutex;
+
+  /// Alarm instance to coalesce progress events.
+  Alarm m_alarm;
 };
 
 } // namespace lldb_private

lldb/source/API/SystemInitializerFull.cpp

@@ -10,6 +10,7 @@
 #include "lldb/API/SBCommandInterpreter.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Progress.h"
 #include "lldb/Host/Config.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Initialization/SystemInitializerCommon.h"
@@ -57,6 +58,7 @@ llvm::Error SystemInitializerFull::Initialize() {
   llvm::InitializeAllAsmPrinters();
   llvm::InitializeAllTargetMCs();
   llvm::InitializeAllDisassemblers();
+
   // Initialize the command line parser in LLVM. This usually isn't necessary
   // as we aren't dealing with command line options here, but otherwise some
   // other code in Clang/LLVM might be tempted to call this function from a
@@ -65,10 +67,13 @@ llvm::Error SystemInitializerFull::Initialize() {
   const char *arg0 = "lldb";
   llvm::cl::ParseCommandLineOptions(1, &arg0);
 
+  // Initialize the progress manager.
+  ProgressManager::Initialize();
+
 #define LLDB_PLUGIN(p) LLDB_PLUGIN_INITIALIZE(p);
 #include "Plugins/Plugins.def"
 
-  // Scan for any system or user LLDB plug-ins
+  // Scan for any system or user LLDB plug-ins.
   PluginManager::Initialize();
 
   // The process settings need to know about installed plug-ins, so the
@@ -84,15 +89,18 @@ llvm::Error SystemInitializerFull::Initialize() {
 void SystemInitializerFull::Terminate() {
   Debugger::SettingsTerminate();
 
-  // Terminate plug-ins in core LLDB
+  // Terminate plug-ins in core LLDB.
   ProcessTrace::Terminate();
 
-  // Terminate and unload and loaded system or user LLDB plug-ins
+  // Terminate and unload and loaded system or user LLDB plug-ins.
   PluginManager::Terminate();
 
 #define LLDB_PLUGIN(p) LLDB_PLUGIN_TERMINATE(p);
 #include "Plugins/Plugins.def"
 
+  // Terminate the progress manager.
+  ProgressManager::Terminate();
+
   // Now shutdown the common parts, in reverse order.
   SystemInitializerCommon::Terminate();
 }

lldb/source/Core/Progress.cpp

@@ -35,7 +35,10 @@ Progress::Progress(std::string title, std::string details,
 
   std::lock_guard<std::mutex> guard(m_mutex);
   ReportProgress();
-  ProgressManager::Instance().Increment(m_progress_data);
+
+  // Report to the ProgressManager if that subsystem is enabled.
+  if (ProgressManager::Enabled())
+    ProgressManager::Instance().Increment(m_progress_data);
 }
 
 Progress::~Progress() {
@@ -45,7 +48,10 @@ Progress::~Progress() {
   if (!m_completed)
     m_completed = m_total;
   ReportProgress();
-  ProgressManager::Instance().Decrement(m_progress_data);
+
+  // Report to the ProgressManager if that subsystem is enabled.
+  if (ProgressManager::Enabled())
+    ProgressManager::Instance().Decrement(m_progress_data);
 }
 
 void Progress::Increment(uint64_t amount,
@@ -75,45 +81,84 @@ void Progress::ReportProgress() {
   }
 }
 
-ProgressManager::ProgressManager() : m_progress_category_map() {}
+ProgressManager::ProgressManager()
+    : m_entries(), m_alarm(std::chrono::milliseconds(100)) {}
 
 ProgressManager::~ProgressManager() {}
 
+void ProgressManager::Initialize() {
+  assert(!InstanceImpl() && "Already initialized.");
+  InstanceImpl().emplace();
+}
+
+void ProgressManager::Terminate() {
+  assert(InstanceImpl() && "Already terminated.");
+  InstanceImpl().reset();
+}
+
+bool ProgressManager::Enabled() { return InstanceImpl().operator bool(); }
+
 ProgressManager &ProgressManager::Instance() {
-  static std::once_flag g_once_flag;
-  static ProgressManager *g_progress_manager = nullptr;
-  std::call_once(g_once_flag, []() {
-    // NOTE: known leak to avoid global destructor chain issues.
-    g_progress_manager = new ProgressManager();
-  });
-  return *g_progress_manager;
+  assert(InstanceImpl() && "ProgressManager must be initialized");
+  return *InstanceImpl();
+}
+
+std::optional<ProgressManager> &ProgressManager::InstanceImpl() {
+  static std::optional<ProgressManager> g_progress_manager;
+  return g_progress_manager;
 }
 
 void ProgressManager::Increment(const Progress::ProgressData &progress_data) {
-  std::lock_guard<std::mutex> lock(m_progress_map_mutex);
-  // If the current category exists in the map then it is not an initial report,
-  // therefore don't broadcast to the category bit. Also, store the current
-  // progress data in the map so that we have a note of the ID used for the
-  // initial progress report.
-  if (!m_progress_category_map.contains(progress_data.title)) {
-    m_progress_category_map[progress_data.title].second = progress_data;
+  std::lock_guard<std::mutex> lock(m_entries_mutex);
+
+  llvm::StringRef key = progress_data.title;
+  bool new_entry = !m_entries.contains(key);
+  Entry &entry = m_entries[progress_data.title];
+
+  if (new_entry) {
+    // This is a new progress event. Report progress and store the progress
+    // data.
     ReportProgress(progress_data, EventType::Begin);
+    entry.data = progress_data;
+  } else if (entry.refcount == 0) {
+    // This is an existing entry that was scheduled to be deleted but a new one
+    // came in before the timer expired.
+    assert(entry.handle != Alarm::INVALID_HANDLE);
+
+    if (!m_alarm.Cancel(entry.handle)) {
+      // The timer expired before we had a chance to cancel it. We have to treat
+      // this as an entirely new progress event.
+      ReportProgress(progress_data, EventType::Begin);
+    }
+    // Clear the alarm handle.
+    entry.handle = Alarm::INVALID_HANDLE;
   }
-  m_progress_category_map[progress_data.title].first++;
+
+  // Regardless of how we got here, we need to bump the reference count.
+  entry.refcount++;
 }
 
 void ProgressManager::Decrement(const Progress::ProgressData &progress_data) {
-  std::lock_guard<std::mutex> lock(m_progress_map_mutex);
-  auto pos = m_progress_category_map.find(progress_data.title);
+  std::lock_guard<std::mutex> lock(m_entries_mutex);
+  llvm::StringRef key = progress_data.title;
 
-  if (pos == m_progress_category_map.end())
+  if (!m_entries.contains(key))
     return;
 
-  if (pos->second.first <= 1) {
-    ReportProgress(pos->second.second, EventType::End);
-    m_progress_category_map.erase(progress_data.title);
-  } else {
-    --pos->second.first;
+  Entry &entry = m_entries[key];
+  entry.refcount--;
+
+  if (entry.refcount == 0) {
+    assert(entry.handle == Alarm::INVALID_HANDLE);
+
+    // Copy the key to a std::string so we can pass it by value to the lambda.
+    // The underlying StringRef will not exist by the time the callback is
+    // called.
+    std::string key_str = std::string(key);
+
+    // Start a timer. If it expires before we see another progress event, it
+    // will be reported.
+    entry.handle = m_alarm.Create([=]() { Expire(key_str); });
   }
 }
 
@@ -129,3 +174,20 @@ void ProgressManager::ReportProgress(
                            progress_data.debugger_id,
                            Debugger::eBroadcastBitProgressCategory);
 }
+
+void ProgressManager::Expire(llvm::StringRef key) {
+  std::lock_guard<std::mutex> lock(m_entries_mutex);
+
+  // This shouldn't happen but be resilient anyway.
+  if (!m_entries.contains(key))
+    return;
+
+  // A new event came in and the alarm fired before we had a chance to restart
+  // it.
+  if (m_entries[key].refcount != 0)
+    return;
+
+  // We're done with this entry.
+  ReportProgress(m_entries[key].data, EventType::End);
+  m_entries.erase(key);
+}

lldb/unittests/Core/ProgressReportTest.cpp

@@ -22,7 +22,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
-static std::chrono::milliseconds TIMEOUT(100);
+static std::chrono::milliseconds TIMEOUT(500);
 
 class ProgressReportTest : public ::testing::Test {
 public:
@@ -56,7 +56,8 @@ class ProgressReportTest : public ::testing::Test {
 
   DebuggerSP m_debugger_sp;
   ListenerSP m_listener_sp;
-  SubsystemRAII<FileSystem, HostInfo, PlatformMacOSX> subsystems;
+  SubsystemRAII<FileSystem, HostInfo, PlatformMacOSX, ProgressManager>
+      subsystems;
 };
 
 TEST_F(ProgressReportTest, TestReportCreation) {
@@ -210,3 +211,37 @@ TEST_F(ProgressReportTest, TestOverlappingEvents) {
   // initial report.
   EXPECT_EQ(data->GetID(), expected_progress_id);
 }
+
+TEST_F(ProgressReportTest, TestProgressManagerDisjointReports) {
+  ListenerSP listener_sp =
+      CreateListenerFor(Debugger::eBroadcastBitProgressCategory);
+  EventSP event_sp;
+  const ProgressEventData *data;
+  uint64_t expected_progress_id;
+
+  { Progress progress("Coalesced report 1", "Starting report 1"); }
+  { Progress progress("Coalesced report 1", "Starting report 2"); }
+  { Progress progress("Coalesced report 1", "Starting report 3"); }
+
+  ASSERT_TRUE(listener_sp->GetEvent(event_sp, TIMEOUT));
+  data = ProgressEventData::GetEventDataFromEvent(event_sp.get());
+  expected_progress_id = data->GetID();
+
+  EXPECT_EQ(data->GetDetails(), "");
+  EXPECT_FALSE(data->IsFinite());
+  EXPECT_FALSE(data->GetCompleted());
+  EXPECT_EQ(data->GetTotal(), Progress::kNonDeterministicTotal);
+  EXPECT_EQ(data->GetMessage(), "Coalesced report 1");
+
+  ASSERT_TRUE(listener_sp->GetEvent(event_sp, TIMEOUT));
+  data = ProgressEventData::GetEventDataFromEvent(event_sp.get());
+
+  EXPECT_EQ(data->GetID(), expected_progress_id);
+  EXPECT_EQ(data->GetDetails(), "");
+  EXPECT_FALSE(data->IsFinite());
+  EXPECT_TRUE(data->GetCompleted());
+  EXPECT_EQ(data->GetTotal(), Progress::kNonDeterministicTotal);
+  EXPECT_EQ(data->GetMessage(), "Coalesced report 1");
+
+  ASSERT_FALSE(listener_sp->GetEvent(event_sp, TIMEOUT));
+}