[lldb] Remove progress report coalescing #130329
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Remove support for coalescing progress reports in LLDB. This functionality was motivated by Xcode, which wanted to listen for less frequent, aggregated progress events at the cost of losing some detail. See the original RFC [1] for more details. Since then, they've reevaluated this trade-off and opted to listen for the regular, full fidelity progress events and do any post processing on their end. rdar://146425487
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesRemove support for coalescing progress reports in LLDB. This functionality was motivated by Xcode, which wanted to listen for less frequent, aggregated progress events at the cost of losing some detail. See the original RFC [1] for more details. Since then, they've reevaluated this trade-off and opted to listen for the regular, full fidelity progress events and do any post processing on their end. rdar://146425487 Patch is 37.32 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/130329.diff 12 Files Affected:
diff --git a/lldb/include/lldb/Core/Progress.h b/lldb/include/lldb/Core/Progress.h
index 5876eae717e96..3003568e8946b 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/Utility/Timeout.h"
#include "lldb/lldb-forward.h"
#include "lldb/lldb-types.h"
@@ -18,6 +17,7 @@
#include <cstdint>
#include <mutex>
#include <optional>
+#include <string>
namespace lldb_private {
@@ -115,21 +115,6 @@ class Progress {
/// Used to indicate a non-deterministic progress report
static constexpr uint64_t kNonDeterministicTotal = UINT64_MAX;
- /// Data belonging to this Progress event that is used for bookkeeping by
- /// ProgressManager.
- struct ProgressData {
- /// The title of the progress activity, also used as a category.
- std::string title;
- /// A unique integer identifier for progress reporting.
- uint64_t progress_id;
- /// The optional debugger ID to report progress to. If this has no value
- /// then all debuggers will receive this event.
- std::optional<lldb::user_id_t> debugger_id;
-
- /// The origin of the progress event, wheter it is internal or external.
- Origin origin;
- };
-
private:
void ReportProgress();
static std::atomic<uint64_t> g_id;
@@ -141,8 +126,18 @@ class Progress {
// Minimum amount of time between two progress reports.
const Timeout<std::nano> m_minimum_report_time;
- /// Data needed by the debugger to broadcast a progress event.
- const ProgressData m_progress_data;
+ /// The title of the progress activity, also used as a category.
+ const std::string m_title;
+
+ /// A unique integer identifier for progress reporting.
+ const uint64_t m_progress_id;
+
+ /// The optional debugger ID to report progress to. If this has no value
+ /// then all debuggers will receive this event.
+ const std::optional<lldb::user_id_t> m_debugger_id;
+
+ /// The origin of the progress event, whether it is internal or external.
+ const Origin m_origin;
/// How much work ([0...m_total]) that has been completed.
std::atomic<uint64_t> m_completed = 0;
@@ -161,60 +156,6 @@ class Progress {
std::optional<uint64_t> m_prev_completed;
};
-/// A class used to group progress reports by category. This is done by using a
-/// map that maintains a refcount of each category of progress reports that have
-/// come in. Keeping track of progress reports this way will be done if a
-/// debugger is listening to the eBroadcastBitProgressByCategory broadcast bit.
-class ProgressManager {
-public:
- ProgressManager();
- ~ProgressManager();
-
- /// Control the refcount of the progress report category as needed.
- void Increment(const Progress::ProgressData &);
- void Decrement(const Progress::ProgressData &);
-
- static void Initialize();
- static void Terminate();
- static bool Enabled();
- static ProgressManager &Instance();
-
-protected:
- 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;
-};
-
} // namespace lldb_private
#endif // LLDB_CORE_PROGRESS_H
diff --git a/lldb/include/lldb/Host/Alarm.h b/lldb/include/lldb/Host/Alarm.h
deleted file mode 100644
index 23b1ff1af5689..0000000000000
--- a/lldb/include/lldb/Host/Alarm.h
+++ /dev/null
@@ -1,115 +0,0 @@
-//===-- Alarm.h -------------------------------------------------*- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef LLDB_HOST_ALARM_H
-#define LLDB_HOST_ALARM_H
-
-#include "lldb/Host/HostThread.h"
-#include "lldb/lldb-types.h"
-#include "llvm/Support/Chrono.h"
-
-#include <condition_variable>
-#include <mutex>
-
-namespace lldb_private {
-
-/// \class Alarm abstraction that enables scheduling a callback function after a
-/// specified timeout. Creating an alarm for a callback returns a Handle that
-/// can be used to restart or cancel the alarm.
-class Alarm {
-public:
- using Handle = uint64_t;
- using Callback = std::function<void()>;
- using TimePoint = llvm::sys::TimePoint<>;
- using Duration = std::chrono::milliseconds;
-
- Alarm(Duration timeout, bool run_callback_on_exit = false);
- ~Alarm();
-
- /// Create an alarm for the given callback. The alarm will expire and the
- /// callback will be called after the timeout.
- ///
- /// \returns
- /// Handle which can be used to restart or cancel the alarm.
- Handle Create(Callback callback);
-
- /// Restart the alarm for the given Handle. The alarm will expire and the
- /// callback will be called after the timeout.
- ///
- /// \returns
- /// True if the alarm was successfully restarted. False if there is no alarm
- /// for the given Handle or the alarm already expired.
- bool Restart(Handle handle);
-
- /// Cancel the alarm for the given Handle. The alarm and its handle will be
- /// removed.
- ///
- /// \returns
- /// True if the alarm was successfully canceled and the Handle removed.
- /// False if there is no alarm for the given Handle or the alarm already
- /// expired.
- bool Cancel(Handle handle);
-
- static constexpr Handle INVALID_HANDLE = 0;
-
-private:
- /// Helper functions to start, stop and check the status of the alarm thread.
- /// @{
- void StartAlarmThread();
- void StopAlarmThread();
- bool AlarmThreadRunning();
- /// @}
-
- /// Return an unique, monotonically increasing handle.
- static Handle GetNextUniqueHandle();
-
- /// Helper to compute the next time the alarm thread needs to wake up.
- TimePoint GetNextExpiration() const;
-
- /// Alarm entry.
- struct Entry {
- Handle handle;
- Callback callback;
- TimePoint expiration;
-
- Entry(Callback callback, TimePoint expiration);
- bool operator==(const Entry &rhs) { return handle == rhs.handle; }
- };
-
- /// List of alarm entries.
- std::vector<Entry> m_entries;
-
- /// Timeout between when an alarm is created and when it fires.
- Duration m_timeout;
-
- /// The alarm thread.
- /// @{
- HostThread m_alarm_thread;
- lldb::thread_result_t AlarmThread();
- /// @}
-
- /// Synchronize access between the alarm thread and the main thread.
- std::mutex m_alarm_mutex;
-
- /// Condition variable used to wake up the alarm thread.
- std::condition_variable m_alarm_cv;
-
- /// Flag to signal the alarm thread that something changed and we need to
- /// recompute the next alarm.
- bool m_recompute_next_alarm = false;
-
- /// Flag to signal the alarm thread to exit.
- bool m_exit = false;
-
- /// Flag to signal we should run all callbacks on exit.
- bool m_run_callbacks_on_exit = false;
-};
-
-} // namespace lldb_private
-
-#endif // LLDB_HOST_ALARM_H
diff --git a/lldb/include/lldb/lldb-enumerations.h b/lldb/include/lldb/lldb-enumerations.h
index fecf9cbb765f7..e30ce5be57b69 100644
--- a/lldb/include/lldb/lldb-enumerations.h
+++ b/lldb/include/lldb/lldb-enumerations.h
@@ -1356,9 +1356,9 @@ enum DebuggerBroadcastBit {
eBroadcastBitWarning = (1 << 1),
eBroadcastBitError = (1 << 2),
eBroadcastSymbolChange = (1 << 3),
- eBroadcastBitProgressCategory = (1 << 4),
+ eBroadcastBitProgressCategory = (1 << 4), ///< Deprecated
eBroadcastBitExternalProgress = (1 << 5),
- eBroadcastBitExternalProgressCategory = (1 << 6),
+ eBroadcastBitExternalProgressCategory = (1 << 6), ///< Deprecated
};
/// Used for expressing severity in logs and diagnostics.
diff --git a/lldb/source/API/SystemInitializerFull.cpp b/lldb/source/API/SystemInitializerFull.cpp
index 31f3a9f30b81f..9cc3779d1895f 100644
--- a/lldb/source/API/SystemInitializerFull.cpp
+++ b/lldb/source/API/SystemInitializerFull.cpp
@@ -68,9 +68,6 @@ llvm::Error SystemInitializerFull::Initialize() {
const char *arg0 = "lldb";
llvm::cl::ParseCommandLineOptions(1, &arg0);
- // Initialize the progress manager.
- ProgressManager::Initialize();
-
#define LLDB_PLUGIN(p) LLDB_PLUGIN_INITIALIZE(p);
#include "Plugins/Plugins.def"
@@ -104,9 +101,6 @@ void SystemInitializerFull::Terminate() {
#define LLDB_PLUGIN(p) LLDB_PLUGIN_TERMINATE(p);
#include "Plugins/Plugins.def"
- // Terminate the progress manager.
- ProgressManager::Terminate();
-
// Now shutdown the common parts, in reverse order.
SystemInitializerCommon::Terminate();
}
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index dbe3d72e5efa4..8c705f889983a 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -1548,7 +1548,7 @@ void Debugger::ReportProgress(uint64_t progress_id, std::string title,
std::string details, uint64_t completed,
uint64_t total,
std::optional<lldb::user_id_t> debugger_id,
- uint32_t progress_category_bit) {
+ uint32_t progress_broadcast_bit) {
// Check if this progress is for a specific debugger.
if (debugger_id) {
// It is debugger specific, grab it and deliver the event if the debugger
@@ -1558,7 +1558,7 @@ void Debugger::ReportProgress(uint64_t progress_id, std::string title,
PrivateReportProgress(*debugger_sp, progress_id, std::move(title),
std::move(details), completed, total,
/*is_debugger_specific*/ true,
- progress_category_bit);
+ progress_broadcast_bit);
return;
}
// The progress event is not debugger specific, iterate over all debuggers
@@ -1569,7 +1569,7 @@ void Debugger::ReportProgress(uint64_t progress_id, std::string title,
for (pos = g_debugger_list_ptr->begin(); pos != end; ++pos)
PrivateReportProgress(*(*pos), progress_id, title, details, completed,
total, /*is_debugger_specific*/ false,
- progress_category_bit);
+ progress_broadcast_bit);
}
}
diff --git a/lldb/source/Core/Progress.cpp b/lldb/source/Core/Progress.cpp
index 63f9804320809..d29dce0a688c0 100644
--- a/lldb/source/Core/Progress.cpp
+++ b/lldb/source/Core/Progress.cpp
@@ -31,11 +31,11 @@ Progress::Progress(std::string title, std::string details,
Timeout<std::nano> minimum_report_time,
Progress::Origin origin)
: m_total(total.value_or(Progress::kNonDeterministicTotal)),
- m_minimum_report_time(minimum_report_time),
- m_progress_data{title, ++g_id,
- debugger ? std::optional<user_id_t>(debugger->GetID())
- : std::nullopt,
- origin},
+ m_minimum_report_time(minimum_report_time), m_title(title),
+ m_progress_id(++g_id),
+ m_debugger_id(debugger ? std::optional<user_id_t>(debugger->GetID())
+ : std::nullopt),
+ m_origin(origin),
m_last_report_time_ns(
std::chrono::nanoseconds(
std::chrono::steady_clock::now().time_since_epoch())
@@ -44,27 +44,19 @@ 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);
-
// Start signpost interval right before the meaningful work starts.
- g_progress_signposts->startInterval(this, m_progress_data.title);
+ g_progress_signposts->startInterval(this, m_title);
}
Progress::~Progress() {
// End signpost interval as soon as possible.
- g_progress_signposts->endInterval(this, m_progress_data.title);
+ g_progress_signposts->endInterval(this, m_title);
// Make sure to always report progress completed when this object is
// destructed so it indicates the progress dialog/activity should go away.
std::lock_guard<std::mutex> guard(m_mutex);
m_completed = m_total;
ReportProgress();
-
- // Report to the ProgressManager if that subsystem is enabled.
- if (ProgressManager::Enabled())
- ProgressManager::Instance().Decrement(m_progress_data);
}
void Progress::Increment(uint64_t amount,
@@ -109,128 +101,11 @@ void Progress::ReportProgress() {
return; // An overflow in the m_completed counter. Just ignore these events.
// Change the category bit if we're an internal or external progress.
- uint32_t progress_category_bit =
- m_progress_data.origin == Progress::Origin::eExternal
- ? lldb::eBroadcastBitExternalProgress
- : lldb::eBroadcastBitProgress;
-
- Debugger::ReportProgress(m_progress_data.progress_id, m_progress_data.title,
- m_details, completed, m_total,
- m_progress_data.debugger_id, progress_category_bit);
- m_prev_completed = completed;
-}
-
-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() {
- 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_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;
- }
-
- // 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_entries_mutex);
- llvm::StringRef key = progress_data.title;
-
- auto it = m_entries.find(key);
- if (it == m_entries.end())
- return;
-
- Entry &entry = it->second;
- 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); });
- }
-}
-
-void ProgressManager::ReportProgress(
- const Progress::ProgressData &progress_data, EventType type) {
- // The category bit only keeps track of when progress report categories have
- // started and ended, so clear the details and reset other fields when
- // broadcasting to it since that bit doesn't need that information.
- const uint64_t completed =
- (type == EventType::Begin) ? 0 : Progress::kNonDeterministicTotal;
- const uint32_t progress_category_bit =
- progress_data.origin == Progress::Origin::eExternal
- ? lldb::eBroadcastBitExternalProgressCategory
- : lldb::eBroadcastBitProgressCategory;
- Debugger::ReportProgress(progress_data.progress_id, progress_data.title, "",
- completed, Progress::kNonDeterministicTotal,
- progress_data.debugger_id, progress_category_bit);
-}
+ uint32_t progress_category_bit = m_origin == Progress::Origin::eExternal
+ ? lldb::eBroadcastBitExternalProgress
+ : lldb::eBroadcastBitProgress;
-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);
+ Debugger::ReportProgress(m_progress_id, m_title, m_details, completed,
+ m_total, m_debugger_id, progress_category_bit);
+ m_prev_completed = completed;
}
diff --git a/lldb/source/Host/CMakeLists.txt b/lldb/source/Host/CMakeLists.txt
index f4be151756b3b..9be0c06a516ba 100644
--- a/lldb/source/Host/CMakeLists.txt
+++ b/lldb/source/Host/CMakeLists.txt
@@ -13,7 +13,6 @@ macro(add_host_subdirectory group)
endmacro()
add_host_subdirectory(common
- common/Alarm.cpp
common/FileAction.cpp
common/FileCache.cpp
common/File.cpp
diff --git a/lldb/source/Host/common/Alarm.cpp b/lldb/source/Host/common/Alarm.cpp
deleted file mode 100644
index afc770d20d7b1..0000000000000
--- a/lldb/source/Host/common/Alarm.cpp
+++ /dev/null
@@ -1,222 +0,0 @@
-//===-- Alarm.cpp ---------------------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#include "lldb/Host/Alarm.h"
-#include "lldb/Host/ThreadLauncher.h"
-#include "lldb/Utility/LLDBLog.h"
-#include "lldb/Utility/Log.h"
-
-using namespace lldb;
-using namespace lldb_private;
-
-Alarm::Alarm(Duration timeout, bool run_callback_on_exit)
- : m_timeout(timeout), m_run_callbacks_on_exit(run_callback_on_exit) {
- StartAlarmThread();
-}
-
-Alarm::~Alarm() { StopAlarmThread(); }
-
-Alarm::Handle Alarm::Create(std::function<void()> callback) {
- // Gracefully deal with the unlikely event that the alarm thread failed to
- // launch.
- if (!AlarmThreadRunning())
- return INVALID_HANDLE;
-
- // Compute the next expiration before we take the lock. This ensures that
- // waiting on the lock doesn't eat into the timeout.
- const TimePoint expiration = GetNextExpiration();
-
- Handle handle = INVALID_HANDLE;
-
- {
- std::lock_guard<std::mutex> alarm_guard(m_alarm_mutex);
-
- // Create a new unique entry and remember its handle.
- m_entries.emplace_back(callback, expiration);
- handle = m_entries.back().handle;
-
- // Tell the alarm thread we need to recompute th...
[truncated]
|
There was a problem hiding this comment.
To my memory the Alarm class and its files were supposed to be fairly general purpose. I know it hasn't been tied in with anything else outside of the coalescing used for progress reports but do you think this class would've been used again or no?
There was a problem hiding this comment.
Possibly, but I don't think it's worth keeping the class around without any uses. If we need something similar in the future, hopefully someone will remember and we can just dig it up from the git history.
There was a problem hiding this comment.
Makes sense, I also figured it could be restored from history if needed if we're not keeping it around, thanks for explaining!
chelcassanova
approved these changes
Mar 7, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Remove support for coalescing progress reports in LLDB. This functionality was motivated by Xcode, which wanted to listen for less frequent, aggregated progress events at the cost of losing some detail. See the original RFC [1] for more details. Since then, they've reevaluated this trade-off and opted to listen for the regular, full fidelity progress events and do any post processing on their end.
rdar://146425487