[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
Jonas Devlieghere via lldb-commits
lldb-commits at lists.llvm.org
Thu Mar 21 10:02:49 PDT 2024
https://github.com/JDevlieghere updated https://github.com/llvm/llvm-project/pull/84854
>From 65d86e85ce27fa4b127bf80fceebf98215b19f64 Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jonas at devlieghere.com>
Date: Wed, 6 Mar 2024 17:22:43 -0800
Subject: [PATCH] [lldb] Implement coalescing of disjoint progress events
This implements coalescing of progress events using a timeout, as
discussed in the RFC on Discourse [1].
[1] https://discourse.llvm.org/t/rfc-improve-lldb-progress-reporting/75717/
---
lldb/include/lldb/Core/Progress.h | 35 +++++-
lldb/source/Core/Progress.cpp | 114 ++++++++++++++----
.../SystemInitializerCommon.cpp | 3 +
lldb/unittests/Core/ProgressReportTest.cpp | 39 +++++-
4 files changed, 159 insertions(+), 32 deletions(-)
diff --git a/lldb/include/lldb/Core/Progress.h b/lldb/include/lldb/Core/Progress.h
index eb3af9816dc6ca..cd87be79c4f0e3 100644
--- a/lldb/include/lldb/Core/Progress.h
+++ b/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,9 +151,12 @@ 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,
@@ -160,9 +164,32 @@ class ProgressManager {
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
diff --git a/lldb/source/Core/Progress.cpp b/lldb/source/Core/Progress.cpp
index b4b5e98b7ba493..161038284e215a 100644
--- a/lldb/source/Core/Progress.cpp
+++ b/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);
+}
diff --git a/lldb/source/Initialization/SystemInitializerCommon.cpp b/lldb/source/Initialization/SystemInitializerCommon.cpp
index 1a172a95aa1471..5d35854211c202 100644
--- a/lldb/source/Initialization/SystemInitializerCommon.cpp
+++ b/lldb/source/Initialization/SystemInitializerCommon.cpp
@@ -9,6 +9,7 @@
#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"
@@ -69,6 +70,7 @@ llvm::Error SystemInitializerCommon::Initialize() {
Diagnostics::Initialize();
FileSystem::Initialize();
HostInfo::Initialize(m_shlib_dir_helper);
+ ProgressManager::Initialize();
llvm::Error error = Socket::Initialize();
if (error)
@@ -97,6 +99,7 @@ void SystemInitializerCommon::Terminate() {
#endif
Socket::Terminate();
+ ProgressManager::Terminate();
HostInfo::Terminate();
Log::DisableAllLogChannels();
FileSystem::Terminate();
diff --git a/lldb/unittests/Core/ProgressReportTest.cpp b/lldb/unittests/Core/ProgressReportTest.cpp
index 1f993180fd8392..f0d253be9bf621 100644
--- a/lldb/unittests/Core/ProgressReportTest.cpp
+++ b/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));
+}
More information about the lldb-commits
mailing list