[Lldb-commits] [lldb] 4dcb1db - Revert "[lldb] Implement coalescing of disjoint progress events (#84854)"
Jonas Devlieghere via lldb-commits
lldb-commits at lists.llvm.org
Mon Mar 25 15:26:27 PDT 2024
Author: Jonas Devlieghere
Date: 2024-03-25T15:25:58-07:00
New Revision: 4dcb1db44f9dbfa09c220703a1b097f51d20a2a5
URL: https://github.com/llvm/llvm-project/commit/4dcb1db44f9dbfa09c220703a1b097f51d20a2a5
DIFF: https://github.com/llvm/llvm-project/commit/4dcb1db44f9dbfa09c220703a1b097f51d20a2a5.diff
LOG: Revert "[lldb] Implement coalescing of disjoint progress events (#84854)"
This reverts commit 930f64689c1fb487714c3836ffa43e49e46aa488 as it's
failing on the Linux bots.
Added:
Modified:
lldb/include/lldb/Core/Progress.h
lldb/source/Core/Progress.cpp
lldb/source/Initialization/SystemInitializerCommon.cpp
lldb/unittests/Core/ProgressReportTest.cpp
Removed:
################################################################################
diff --git a/lldb/include/lldb/Core/Progress.h b/lldb/include/lldb/Core/Progress.h
index cd87be79c4f0e3..eb3af9816dc6ca 100644
--- a/lldb/include/lldb/Core/Progress.h
+++ b/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,12 +150,9 @@ 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,
@@ -164,32 +160,9 @@ class ProgressManager {
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
diff --git a/lldb/source/Core/Progress.cpp b/lldb/source/Core/Progress.cpp
index 161038284e215a..b4b5e98b7ba493 100644
--- a/lldb/source/Core/Progress.cpp
+++ b/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);
-}
diff --git a/lldb/source/Initialization/SystemInitializerCommon.cpp b/lldb/source/Initialization/SystemInitializerCommon.cpp
index 5d35854211c202..1a172a95aa1471 100644
--- a/lldb/source/Initialization/SystemInitializerCommon.cpp
+++ b/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();
diff --git a/lldb/unittests/Core/ProgressReportTest.cpp b/lldb/unittests/Core/ProgressReportTest.cpp
index f0d253be9bf621..1f993180fd8392 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(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));
-}
More information about the lldb-commits
mailing list