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