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 <jo...@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)); +} _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
