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

This reverts commit 930f646 as it's
failing on the Linux bots.

Author: JDevlieghere

lldb/include/lldb/Core/Progress.h

@@ -9,7 +9,6 @@
 #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"
@@ -151,45 +150,19 @@ class ProgressManager {
   void Increment(const Progress::ProgressData &);
   void Decrement(const Progress::ProgressData &);
 
-  static void Initialize();
-  static void Terminate();
-  static bool Enabled();
   static ProgressManager &Instance();
 
-protected:
+private:
   enum class EventType {
     Begin,
     End,
   };
   static void ReportProgress(const Progress::ProgressData &progress_data,
                              EventType type);
 
-  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;
+  llvm::StringMap<std::pair<uint64_t, Progress::ProgressData>>
+      m_progress_category_map;
+  std::mutex m_progress_map_mutex;
 };
 
 } // namespace lldb_private

lldb/source/Core/Progress.cpp

@@ -35,10 +35,7 @@ Progress::Progress(std::string title, std::string details,
 
   std::lock_guard<std::mutex> guard(m_mutex);
   ReportProgress();
-
-  // Report to the ProgressManager if that subsystem is enabled.
-  if (ProgressManager::Enabled())
-    ProgressManager::Instance().Increment(m_progress_data);
+  ProgressManager::Instance().Increment(m_progress_data);
 }
 
 Progress::~Progress() {
@@ -48,10 +45,7 @@ Progress::~Progress() {
   if (!m_completed)
     m_completed = m_total;
   ReportProgress();
-
-  // Report to the ProgressManager if that subsystem is enabled.
-  if (ProgressManager::Enabled())
-    ProgressManager::Instance().Decrement(m_progress_data);
+  ProgressManager::Instance().Decrement(m_progress_data);
 }
 
 void Progress::Increment(uint64_t amount,
@@ -81,84 +75,45 @@ void Progress::ReportProgress() {
   }
 }
 
-ProgressManager::ProgressManager()
-    : m_entries(), m_alarm(std::chrono::milliseconds(100)) {}
+ProgressManager::ProgressManager() : m_progress_category_map() {}
 
 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() {
-  assert(InstanceImpl() && "ProgressManager must be initialized");
-  return *InstanceImpl();
-}
-
-std::optional<ProgressManager> &ProgressManager::InstanceImpl() {
-  static std::optional<ProgressManager> g_progress_manager;
-  return g_progress_manager;
+  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;
 }
 
 void ProgressManager::Increment(const Progress::ProgressData &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.
+  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;
     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;
   }
-
-  // Regardless of how we got here, we need to bump the reference count.
-  entry.refcount++;
+  m_progress_category_map[progress_data.title].first++;
 }
 
 void ProgressManager::Decrement(const Progress::ProgressData &progress_data) {
-  std::lock_guard<std::mutex> lock(m_entries_mutex);
-  llvm::StringRef key = progress_data.title;
+  std::lock_guard<std::mutex> lock(m_progress_map_mutex);
+  auto pos = m_progress_category_map.find(progress_data.title);
 
-  if (!m_entries.contains(key))
+  if (pos == m_progress_category_map.end())
     return;
 
-  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); });
+  if (pos->second.first <= 1) {
+    ReportProgress(pos->second.second, EventType::End);
+    m_progress_category_map.erase(progress_data.title);
+  } else {
+    --pos->second.first;
   }
 }
 
@@ -174,20 +129,3 @@ 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/source/Initialization/SystemInitializerCommon.cpp

@@ -9,7 +9,6 @@
 #include "lldb/Initialization/SystemInitializerCommon.h"
 
 #include "Plugins/Process/gdb-remote/ProcessGDBRemoteLog.h"
-#include "lldb/Core/Progress.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/Socket.h"
@@ -70,7 +69,6 @@ llvm::Error SystemInitializerCommon::Initialize() {
   Diagnostics::Initialize();
   FileSystem::Initialize();
   HostInfo::Initialize(m_shlib_dir_helper);
-  ProgressManager::Initialize();
 
   llvm::Error error = Socket::Initialize();
   if (error)
@@ -99,7 +97,6 @@ void SystemInitializerCommon::Terminate() {
 #endif
 
   Socket::Terminate();
-  ProgressManager::Terminate();
   HostInfo::Terminate();
   Log::DisableAllLogChannels();
   FileSystem::Terminate();

lldb/unittests/Core/ProgressReportTest.cpp

@@ -22,7 +22,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
-static std::chrono::milliseconds TIMEOUT(500);
+static std::chrono::milliseconds TIMEOUT(100);
 
 class ProgressReportTest : public ::testing::Test {
 public:
@@ -56,8 +56,7 @@ class ProgressReportTest : public ::testing::Test {
 
   DebuggerSP m_debugger_sp;
   ListenerSP m_listener_sp;
-  SubsystemRAII<FileSystem, HostInfo, PlatformMacOSX, ProgressManager>
-      subsystems;
+  SubsystemRAII<FileSystem, HostInfo, PlatformMacOSX> subsystems;
 };
 
 TEST_F(ProgressReportTest, TestReportCreation) {
@@ -211,37 +210,3 @@ 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));
-}