[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
https://github.com/JDevlieghere created
https://github.com/llvm/llvm-project/pull/84854
This implements coalescing of progress events using a timeout, as discussed in
the RFC on Discourse [1]. This PR consists of two commits which, depending on
the feedback, I may split up into two PRs. For now, I think it's easier to
review this as a whole.
1. The first commit introduces a new generic `Alarm` class. The class lets you
to schedule a function (callback) to be executed after a given timeout expires.
You can cancel and reset a callback before its corresponding timeout expires.
It achieves this with the help of a worker thread that sleeps until the next
timeout expires. The only guarantee it provides is that your function is called
no sooner than the requested timeout. Because the callback is called directly
from the worker thread, a long running callback could potentially block the
worker thread. I intentionally kept the implementation as simple as possible
while addressing the needs for the `ProgressManager` use case. If we want to
rely on this somewhere else, we can reassess whether we need to address those
limitations.
2. The second commit uses the Alarm class to coalesce progress events. To recap
the Discourse discussion, when multiple progress events with the same title
execute in close succession, they get broadcast as one to
`eBroadcastBitProgressCategory`. The `ProgressManager` keeps track of the
in-flight progress events and when the refcount hits zero, the Alarm class is
used to schedule broadcasting the event. If a new progress event comes in
before the alarm fires, the alarm is reset (and the process repeats when the
new progress event ends). If no new event comes in before the timeout expires,
the progress event is broadcast.
[1] https://discourse.llvm.org/t/rfc-improve-lldb-progress-reporting/75717/
>From 3aae84b8caaf904c11b1dab620893620a0fa1c85 Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere
Date: Tue, 5 Mar 2024 22:57:43 -0800
Subject: [PATCH 1/2] [lldb] Add an Alarm class
Add an Alarm class which allows scheduling callbacks after a specific
timeout has elapsed.
---
lldb/include/lldb/Host/Alarm.h | 88 +
lldb/source/Host/CMakeLists.txt| 1 +
lldb/source/Host/common/Alarm.cpp | 193 +
lldb/unittests/Host/AlarmTest.cpp | 164
lldb/unittests/Host/CMakeLists.txt | 1 +
5 files changed, 447 insertions(+)
create mode 100644 lldb/include/lldb/Host/Alarm.h
create mode 100644 lldb/source/Host/common/Alarm.cpp
create mode 100644 lldb/unittests/Host/AlarmTest.cpp
diff --git a/lldb/include/lldb/Host/Alarm.h b/lldb/include/lldb/Host/Alarm.h
new file mode 100644
index 00..8b4180fbb32835
--- /dev/null
+++ b/lldb/include/lldb/Host/Alarm.h
@@ -0,0 +1,88 @@
+//===-- 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"
+
+namespace lldb_private {
+
+class Alarm {
+public:
+ using Handle = uint64_t;
+ using Callback = std::function;
+ using TimePoint = llvm::sys::TimePoint<>;
+ using Duration = std::chrono::milliseconds;
+
+ Alarm(Duration timeout, bool run_callback_on_exit = false);
+ ~Alarm();
+
+ Handle Create(Callback callback);
+ bool Restart(Handle handle);
+ 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();
+
+ 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 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_ala
[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
llvmbot wrote:
@llvm/pr-subscribers-lldb
Author: Jonas Devlieghere (JDevlieghere)
Changes
This implements coalescing of progress events using a timeout, as discussed in
the RFC on Discourse [1]. This PR consists of two commits which, depending on
the feedback, I may split up into two PRs. For now, I think it's easier to
review this as a whole.
1. The first commit introduces a new generic `Alarm` class. The class lets you
to schedule a function (callback) to be executed after a given timeout expires.
You can cancel and reset a callback before its corresponding timeout expires.
It achieves this with the help of a worker thread that sleeps until the next
timeout expires. The only guarantee it provides is that your function is called
no sooner than the requested timeout. Because the callback is called directly
from the worker thread, a long running callback could potentially block the
worker thread. I intentionally kept the implementation as simple as possible
while addressing the needs for the `ProgressManager` use case. If we want to
rely on this somewhere else, we can reassess whether we need to address those
limitations.
2. The second commit uses the Alarm class to coalesce progress events. To recap
the Discourse discussion, when multiple progress events with the same title
execute in close succession, they get broadcast as one to
`eBroadcastBitProgressCategory`. The `ProgressManager` keeps track of the
in-flight progress events and when the refcount hits zero, the Alarm class is
used to schedule broadcasting the event. If a new progress event comes in
before the alarm fires, the alarm is reset (and the process repeats when the
new progress event ends). If no new event comes in before the timeout expires,
the progress event is broadcast.
[1] https://discourse.llvm.org/t/rfc-improve-lldb-progress-reporting/75717/
---
Patch is 24.79 KiB, truncated to 20.00 KiB below, full version:
https://github.com/llvm/llvm-project/pull/84854.diff
9 Files Affected:
- (modified) lldb/include/lldb/Core/Progress.h (+17-4)
- (added) lldb/include/lldb/Host/Alarm.h (+88)
- (modified) lldb/source/Core/Progress.cpp (+89-25)
- (modified) lldb/source/Host/CMakeLists.txt (+1)
- (added) lldb/source/Host/common/Alarm.cpp (+193)
- (modified) lldb/source/Initialization/SystemInitializerCommon.cpp (+3)
- (modified) lldb/unittests/Core/ProgressReportTest.cpp (+54-2)
- (added) lldb/unittests/Host/AlarmTest.cpp (+164)
- (modified) lldb/unittests/Host/CMakeLists.txt (+1)
``diff
diff --git a/lldb/include/lldb/Core/Progress.h
b/lldb/include/lldb/Core/Progress.h
index c38f6dd0a140ed..67d96b01e67c4a 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"
@@ -146,9 +147,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,
@@ -156,9 +160,18 @@ class ProgressManager {
static void ReportProgress(const Progress::ProgressData &progress_data,
EventType type);
- llvm::StringMap>
- m_progress_category_map;
- std::mutex m_progress_map_mutex;
+ static std::optional &InstanceImpl();
+
+ void Expire(llvm::StringRef key);
+ struct Entry {
+uint64_t refcount = 0;
+Progress::ProgressData data;
+Alarm::Handle handle = Alarm::INVALID_HANDLE;
+ };
+
+ Alarm m_alarm;
+ llvm::StringMap m_entries;
+ std::mutex m_entries_mutex;
};
} // namespace lldb_private
diff --git a/lldb/include/lldb/Host/Alarm.h b/lldb/include/lldb/Host/Alarm.h
new file mode 100644
index 00..8b4180fbb32835
--- /dev/null
+++ b/lldb/include/lldb/Host/Alarm.h
@@ -0,0 +1,88 @@
+//===-- 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"
+
+namespace lldb_private {
+
+class Alarm {
+public:
+ using Handle = uint64_t;
+ using Callback = std::function;
+ using TimePoint = llvm::sys::TimePoint<>;
+ using Duration = std::chrono::milliseconds;
+
+ Alarm(Duration timeout, bool run_callback_on_exit = false);
+ ~Alarm();
+
+ Handle Create(Callback callback);
+ bool Restart(Handle handle);
+ bool Canc
[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
https://github.com/JDevlieghere updated
https://github.com/llvm/llvm-project/pull/84854
>From 3aae84b8caaf904c11b1dab620893620a0fa1c85 Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere
Date: Tue, 5 Mar 2024 22:57:43 -0800
Subject: [PATCH 1/2] [lldb] Add an Alarm class
Add an Alarm class which allows scheduling callbacks after a specific
timeout has elapsed.
---
lldb/include/lldb/Host/Alarm.h | 88 +
lldb/source/Host/CMakeLists.txt| 1 +
lldb/source/Host/common/Alarm.cpp | 193 +
lldb/unittests/Host/AlarmTest.cpp | 164
lldb/unittests/Host/CMakeLists.txt | 1 +
5 files changed, 447 insertions(+)
create mode 100644 lldb/include/lldb/Host/Alarm.h
create mode 100644 lldb/source/Host/common/Alarm.cpp
create mode 100644 lldb/unittests/Host/AlarmTest.cpp
diff --git a/lldb/include/lldb/Host/Alarm.h b/lldb/include/lldb/Host/Alarm.h
new file mode 100644
index 00..8b4180fbb32835
--- /dev/null
+++ b/lldb/include/lldb/Host/Alarm.h
@@ -0,0 +1,88 @@
+//===-- 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"
+
+namespace lldb_private {
+
+class Alarm {
+public:
+ using Handle = uint64_t;
+ using Callback = std::function;
+ using TimePoint = llvm::sys::TimePoint<>;
+ using Duration = std::chrono::milliseconds;
+
+ Alarm(Duration timeout, bool run_callback_on_exit = false);
+ ~Alarm();
+
+ Handle Create(Callback callback);
+ bool Restart(Handle handle);
+ 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();
+
+ 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 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/source/Host/CMakeLists.txt b/lldb/source/Host/CMakeLists.txt
index fe6e539f758fda..c2e091ee8555b7 100644
--- a/lldb/source/Host/CMakeLists.txt
+++ b/lldb/source/Host/CMakeLists.txt
@@ -13,6 +13,7 @@ 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
new file mode 100644
index 00..b9366522210361
--- /dev/null
+++ b/lldb/source/Host/common/Alarm.cpp
@@ -0,0 +1,193 @@
+//===-- 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 callback) {
+ // Gracefully deal with the unlikely event that the alarm thread failed to
+ // launch.
+ if
[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
@@ -0,0 +1,88 @@
+//===-- 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"
+
+namespace lldb_private {
+
+class Alarm {
+public:
+ using Handle = uint64_t;
+ using Callback = std::function;
+ using TimePoint = llvm::sys::TimePoint<>;
+ using Duration = std::chrono::milliseconds;
+
+ Alarm(Duration timeout, bool run_callback_on_exit = false);
+ ~Alarm();
+
+ Handle Create(Callback callback);
+ bool Restart(Handle handle);
+ 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();
+
+ 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 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.
chelcassanova wrote:
nit: missing third slash for doxygen?
https://github.com/llvm/llvm-project/pull/84854
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
@@ -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));
chelcassanova wrote:
Good point to try popping another event from the stack and expect that to not
work, this was something I was going to add to the original progress manager
test but I thought would be overkill 😅 .
https://github.com/llvm/llvm-project/pull/84854
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
@@ -0,0 +1,164 @@
+//===-- AlarmTest.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 "gtest/gtest.h"
+
+#include
+#include
+
+using namespace lldb_private;
+using namespace std::chrono_literals;
+
+static constexpr auto ALARM_TIMEOUT = 500ms;
+static constexpr auto TEST_TIMEOUT = 1000ms;
+static constexpr bool RUN_CALLBACKS_ON_EXIT = true;
+
+// Enable strict checking of the ALARM_TIMEOUTs. This should only be enabled
for
+// development and never during automated testing where scheduling and
+// ALARM_TIMEOUTs are unpredictable.
+#define STRICT_ALARM_TIMEOUT 1
+
+#if STRICT_ALARM_TIMEOUT
+static constexpr auto EPSILON = 10ms;
+#endif
+
+TEST(AlarmTest, Create) {
+ std::mutex m;
+
+ std::vector callbacks_actual;
+ std::vector callbacks_expected;
+
+ Alarm alarm(ALARM_TIMEOUT, RUN_CALLBACKS_ON_EXIT);
+
+ // Create 5 alarms some time apart.
+ for (size_t i = 0; i < 5; ++i) {
+callbacks_actual.emplace_back();
+callbacks_expected.emplace_back(std::chrono::system_clock::now() +
+ALARM_TIMEOUT);
+
+alarm.Create([&callbacks_actual, &m, i]() {
+ std::lock_guard guard(m);
+ callbacks_actual[i] = std::chrono::system_clock::now();
+});
+
+std::this_thread::sleep_for(ALARM_TIMEOUT / 5);
+ }
+
+ // Leave plenty of time for all the alarms to fire.
+ std::this_thread::sleep_for(TEST_TIMEOUT);
+
+ // Make sure all the alarms around the expected time.
chelcassanova wrote:
nit: Missing word or am I just reading this wrong?
https://github.com/llvm/llvm-project/pull/84854
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
@@ -0,0 +1,164 @@
+//===-- AlarmTest.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 "gtest/gtest.h"
+
+#include
+#include
+
+using namespace lldb_private;
+using namespace std::chrono_literals;
+
+static constexpr auto ALARM_TIMEOUT = 500ms;
+static constexpr auto TEST_TIMEOUT = 1000ms;
+static constexpr bool RUN_CALLBACKS_ON_EXIT = true;
+
+// Enable strict checking of the ALARM_TIMEOUTs. This should only be enabled
for
+// development and never during automated testing where scheduling and
+// ALARM_TIMEOUTs are unpredictable.
+#define STRICT_ALARM_TIMEOUT 1
+
+#if STRICT_ALARM_TIMEOUT
+static constexpr auto EPSILON = 10ms;
+#endif
+
+TEST(AlarmTest, Create) {
+ std::mutex m;
+
+ std::vector callbacks_actual;
+ std::vector callbacks_expected;
+
+ Alarm alarm(ALARM_TIMEOUT, RUN_CALLBACKS_ON_EXIT);
+
+ // Create 5 alarms some time apart.
+ for (size_t i = 0; i < 5; ++i) {
+callbacks_actual.emplace_back();
+callbacks_expected.emplace_back(std::chrono::system_clock::now() +
+ALARM_TIMEOUT);
+
+alarm.Create([&callbacks_actual, &m, i]() {
+ std::lock_guard guard(m);
+ callbacks_actual[i] = std::chrono::system_clock::now();
+});
+
+std::this_thread::sleep_for(ALARM_TIMEOUT / 5);
+ }
+
+ // Leave plenty of time for all the alarms to fire.
+ std::this_thread::sleep_for(TEST_TIMEOUT);
+
+ // Make sure all the alarms around the expected time.
+ for (size_t i = 0; i < 5; ++i) {
+EXPECT_GE(callbacks_actual[i], callbacks_expected[i]);
+#if STRICT_ALARM_TIMEOUT
+EXPECT_LE(callbacks_actual[i], callbacks_expected[i] + EPSILON);
+#endif
+ }
+}
+
+TEST(AlarmTest, Exit) {
+ std::mutex m;
+
+ std::vector handles;
+ std::vector callbacks;
+
+ {
+Alarm alarm(ALARM_TIMEOUT, RUN_CALLBACKS_ON_EXIT);
+
+// Create 5 alarms.
+for (size_t i = 0; i < 5; ++i) {
+ callbacks.emplace_back(false);
+
+ handles.push_back(alarm.Create([&callbacks, &m, i]() {
+std::lock_guard guard(m);
+callbacks[i] = true;
+ }));
+}
+
+// Let the alarm go out of scope before any alarm had a chance to fire.
+ }
+
+ // Make sure none of the first 4 alarms fired.
+ for (bool callback : callbacks)
+EXPECT_TRUE(callback);
chelcassanova wrote:
Just wondering, why do we check that the first 4 alarms didn't fire but not the
final one here?
https://github.com/llvm/llvm-project/pull/84854
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
@@ -0,0 +1,88 @@
+//===-- 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"
+
+namespace lldb_private {
+
+class Alarm {
+public:
+ using Handle = uint64_t;
+ using Callback = std::function;
+ using TimePoint = llvm::sys::TimePoint<>;
+ using Duration = std::chrono::milliseconds;
+
+ Alarm(Duration timeout, bool run_callback_on_exit = false);
+ ~Alarm();
+
+ Handle Create(Callback callback);
adrian-prantl wrote:
Maybe in the toplevel doxygen comment explain what the expected flow is, ie,
that one Alarm can deal with multiple handles and what the expected behavior is?
https://github.com/llvm/llvm-project/pull/84854
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
@@ -0,0 +1,88 @@
+//===-- 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"
+
+namespace lldb_private {
+
+class Alarm {
adrian-prantl wrote:
Can you add Doxygen comment explaining what this is to be used for?
https://github.com/llvm/llvm-project/pull/84854
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
@@ -146,19 +147,31 @@ 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,
};
static void ReportProgress(const Progress::ProgressData &progress_data,
EventType type);
- llvm::StringMap>
- m_progress_category_map;
- std::mutex m_progress_map_mutex;
+ static std::optional &InstanceImpl();
+
+ void Expire(llvm::StringRef key);
adrian-prantl wrote:
Can you add a Doxygen comment?
https://github.com/llvm/llvm-project/pull/84854
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
@@ -146,19 +147,31 @@ 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,
};
static void ReportProgress(const Progress::ProgressData &progress_data,
EventType type);
- llvm::StringMap>
- m_progress_category_map;
- std::mutex m_progress_map_mutex;
+ static std::optional &InstanceImpl();
+
+ void Expire(llvm::StringRef key);
+ struct Entry {
adrian-prantl wrote:
same here
https://github.com/llvm/llvm-project/pull/84854
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
@@ -75,45 +81,86 @@ void Progress::ReportProgress() {
}
}
-ProgressManager::ProgressManager() : m_progress_category_map() {}
+ProgressManager::ProgressManager()
+: m_alarm(std::chrono::milliseconds(100)), m_entries() {}
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::InstanceImpl() {
+ static std::optional g_progress_manager;
+ return g_progress_manager;
}
void ProgressManager::Increment(const Progress::ProgressData &progress_data) {
- std::lock_guard 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 lock(m_entries_mutex);
+ llvm::StringRef key = progress_data.title;
+
+ // This is a new progress event.
+ if (!m_entries.contains(key)) {
ReportProgress(progress_data, EventType::Begin);
+Entry &entry = m_entries[key];
+entry.data = progress_data;
+entry.refcount = 1;
+return;
+ }
+
+ // This is an existing progress event.
+ Entry &entry = m_entries[key];
clayborg wrote:
Since we are always calling `m_entries[key]` anyway, this code could be a bit
simpler:
```
Entry &entry = m_entries[key];
if (entry.refcount == 0) {
// This is a new progress event.
entry.data = progress_data;
entry.refcount = 1;
return;
}
// This is an existing progress event.
```
I assume that accessing a non existent key in the string map will return an
default constructed object right?
https://github.com/llvm/llvm-project/pull/84854
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
@@ -75,45 +81,86 @@ void Progress::ReportProgress() {
}
}
-ProgressManager::ProgressManager() : m_progress_category_map() {}
+ProgressManager::ProgressManager()
+: m_alarm(std::chrono::milliseconds(100)), m_entries() {}
clayborg wrote:
Make this a lldb setting instead of hard coding to 100ms?
https://github.com/llvm/llvm-project/pull/84854
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
@@ -75,45 +81,86 @@ void Progress::ReportProgress() {
}
}
-ProgressManager::ProgressManager() : m_progress_category_map() {}
+ProgressManager::ProgressManager()
+: m_alarm(std::chrono::milliseconds(100)), m_entries() {}
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::InstanceImpl() {
+ static std::optional g_progress_manager;
+ return g_progress_manager;
}
void ProgressManager::Increment(const Progress::ProgressData &progress_data) {
- std::lock_guard 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 lock(m_entries_mutex);
+ llvm::StringRef key = progress_data.title;
+
+ // This is a new progress event.
+ if (!m_entries.contains(key)) {
ReportProgress(progress_data, EventType::Begin);
+Entry &entry = m_entries[key];
+entry.data = progress_data;
+entry.refcount = 1;
+return;
+ }
+
+ // This is an existing progress event.
+ Entry &entry = m_entries[key];
+
+ // The progress event was scheduled to be deleted but a new one came in
before
+ // the timer expired.
+ if (entry.refcount == 0) {
+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);
+}
+entry.refcount = 1;
+entry.handle = Alarm::INVALID_HANDLE;
+ } else {
+entry.refcount++;
}
clayborg wrote:
One idea for this code could be:
```
if (entry.refcount++ == 1) {
// This entry was scheduled to be expired, but no longer is.
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);
}
entry.handle = Alarm::INVALID_HANDLE;
}
```
This just removes the need to manually set `entry.refcount = 1;` or to
separately increment refcount in the else body
https://github.com/llvm/llvm-project/pull/84854
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
@@ -75,45 +81,86 @@ void Progress::ReportProgress() {
}
}
-ProgressManager::ProgressManager() : m_progress_category_map() {}
+ProgressManager::ProgressManager()
+: m_alarm(std::chrono::milliseconds(100)), m_entries() {}
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::InstanceImpl() {
+ static std::optional g_progress_manager;
+ return g_progress_manager;
}
void ProgressManager::Increment(const Progress::ProgressData &progress_data) {
- std::lock_guard 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 lock(m_entries_mutex);
+ llvm::StringRef key = progress_data.title;
+
+ // This is a new progress event.
+ if (!m_entries.contains(key)) {
ReportProgress(progress_data, EventType::Begin);
+Entry &entry = m_entries[key];
+entry.data = progress_data;
+entry.refcount = 1;
+return;
+ }
+
+ // This is an existing progress event.
+ Entry &entry = m_entries[key];
+
+ // The progress event was scheduled to be deleted but a new one came in
before
+ // the timer expired.
+ if (entry.refcount == 0) {
+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);
+}
+entry.refcount = 1;
+entry.handle = Alarm::INVALID_HANDLE;
+ } else {
+entry.refcount++;
}
- m_progress_category_map[progress_data.title].first++;
}
void ProgressManager::Decrement(const Progress::ProgressData &progress_data) {
- std::lock_guard lock(m_progress_map_mutex);
- auto pos = m_progress_category_map.find(progress_data.title);
+ std::lock_guard 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);
+ Entry &entry = m_entries[key];
+ if (entry.refcount <= 1) {
+assert(entry.handle == Alarm::INVALID_HANDLE);
+// Start a timer. If it expires before we see another progress event, it
+// will be reported.
+entry.refcount = 0;
+// 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);
+entry.handle = m_alarm.Create([=]() { Expire(key_str); });
} else {
---pos->second.first;
+entry.refcount--;
}
clayborg wrote:
This can similarly always decrement, similar to previous suggestion:
```
Entry &entry = m_entries[key];
if (entry.refcount > 0 && --entry.refcount == 0) {
assert(entry.handle == Alarm::INVALID_HANDLE);
// Start a timer. If it expires before we see another progress event, it
// will be reported.
// 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);
entry.handle = m_alarm.Create([=]() { Expire(key_str); });
}
```
This stops the manual editing of `entry.refcount = 0;` and the separate
decrement in the else
https://github.com/llvm/llvm-project/pull/84854
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
https://github.com/JDevlieghere updated
https://github.com/llvm/llvm-project/pull/84854
>From aae699eb956d1e235682b34e6407f6a9990028b3 Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere
Date: Tue, 5 Mar 2024 22:57:43 -0800
Subject: [PATCH 1/2] [lldb] Add an Alarm class
Add an Alarm class which allows scheduling callbacks after a specific
timeout has elapsed.
---
lldb/include/lldb/Host/Alarm.h | 112 +
lldb/source/Host/CMakeLists.txt| 1 +
lldb/source/Host/common/Alarm.cpp | 193 +
lldb/unittests/Host/AlarmTest.cpp | 164
lldb/unittests/Host/CMakeLists.txt | 1 +
5 files changed, 471 insertions(+)
create mode 100644 lldb/include/lldb/Host/Alarm.h
create mode 100644 lldb/source/Host/common/Alarm.cpp
create mode 100644 lldb/unittests/Host/AlarmTest.cpp
diff --git a/lldb/include/lldb/Host/Alarm.h b/lldb/include/lldb/Host/Alarm.h
new file mode 100644
index 00..7bdbc8f2b0ed74
--- /dev/null
+++ b/lldb/include/lldb/Host/Alarm.h
@@ -0,0 +1,112 @@
+//===-- 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"
+
+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;
+ 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 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/source/Host/CMakeLists.txt b/lldb/source/Host/CMakeLists.txt
index fe6e539f758fda..c2e091ee8555b7 100644
--- a/lldb/source/Host/CMakeLists.txt
+++ b/lldb/source/Host/CMakeLists.txt
@@ -13,6 +13,7 @@ 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-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
@@ -0,0 +1,164 @@
+//===-- AlarmTest.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 "gtest/gtest.h"
+
+#include
+#include
+
+using namespace lldb_private;
+using namespace std::chrono_literals;
+
+static constexpr auto ALARM_TIMEOUT = 500ms;
+static constexpr auto TEST_TIMEOUT = 1000ms;
+static constexpr bool RUN_CALLBACKS_ON_EXIT = true;
+
+// Enable strict checking of the ALARM_TIMEOUTs. This should only be enabled
for
+// development and never during automated testing where scheduling and
+// ALARM_TIMEOUTs are unpredictable.
+#define STRICT_ALARM_TIMEOUT 1
+
+#if STRICT_ALARM_TIMEOUT
+static constexpr auto EPSILON = 10ms;
+#endif
+
+TEST(AlarmTest, Create) {
+ std::mutex m;
+
+ std::vector callbacks_actual;
+ std::vector callbacks_expected;
+
+ Alarm alarm(ALARM_TIMEOUT, RUN_CALLBACKS_ON_EXIT);
+
+ // Create 5 alarms some time apart.
+ for (size_t i = 0; i < 5; ++i) {
+callbacks_actual.emplace_back();
+callbacks_expected.emplace_back(std::chrono::system_clock::now() +
+ALARM_TIMEOUT);
+
+alarm.Create([&callbacks_actual, &m, i]() {
+ std::lock_guard guard(m);
+ callbacks_actual[i] = std::chrono::system_clock::now();
+});
+
+std::this_thread::sleep_for(ALARM_TIMEOUT / 5);
+ }
+
+ // Leave plenty of time for all the alarms to fire.
+ std::this_thread::sleep_for(TEST_TIMEOUT);
+
+ // Make sure all the alarms around the expected time.
+ for (size_t i = 0; i < 5; ++i) {
+EXPECT_GE(callbacks_actual[i], callbacks_expected[i]);
+#if STRICT_ALARM_TIMEOUT
+EXPECT_LE(callbacks_actual[i], callbacks_expected[i] + EPSILON);
+#endif
+ }
+}
+
+TEST(AlarmTest, Exit) {
+ std::mutex m;
+
+ std::vector handles;
+ std::vector callbacks;
+
+ {
+Alarm alarm(ALARM_TIMEOUT, RUN_CALLBACKS_ON_EXIT);
+
+// Create 5 alarms.
+for (size_t i = 0; i < 5; ++i) {
+ callbacks.emplace_back(false);
+
+ handles.push_back(alarm.Create([&callbacks, &m, i]() {
+std::lock_guard guard(m);
+callbacks[i] = true;
+ }));
+}
+
+// Let the alarm go out of scope before any alarm had a chance to fire.
+ }
+
+ // Make sure none of the first 4 alarms fired.
+ for (bool callback : callbacks)
+EXPECT_TRUE(callback);
JDevlieghere wrote:
Yup, that's a typo.
https://github.com/llvm/llvm-project/pull/84854
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
@@ -75,45 +81,86 @@ void Progress::ReportProgress() {
}
}
-ProgressManager::ProgressManager() : m_progress_category_map() {}
+ProgressManager::ProgressManager()
+: m_alarm(std::chrono::milliseconds(100)), m_entries() {}
JDevlieghere wrote:
I initially wanted to make this a setting, but you need this when initializing
the ProgressManager, which happens way before we even have a debugger or a
command interpreter, so there wouldn't be a way to set the setting even. I
could move the timeout out of the constructor and allow you to set the timeout
after the fact, but that would still leave the question of where to get the
setting from: the ProgressManager is shared across debuggers.
https://github.com/llvm/llvm-project/pull/84854
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
https://github.com/chelcassanova approved this pull request. https://github.com/llvm/llvm-project/pull/84854 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
JDevlieghere wrote: Seems like the alarm part of this patch is pretty uncontroversial so I'm going to go ahead and split that off into its own PR so we can settle on the ProgressManager part here. https://github.com/llvm/llvm-project/pull/84854 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
https://github.com/adrian-prantl approved this pull request. https://github.com/llvm/llvm-project/pull/84854 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
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
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>
- m_progress_category_map;
- std::mutex m_progress_map_mutex;
+ static std::optional &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 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 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::InstanceImpl() {
+ static std::optional g_progress_manager;
+ return g_progress_manager;
}
void ProgressManager::Increment(const Progress::ProgressData &progress_data) {
- std::lock_guard 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 i
[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
JDevlieghere wrote: Rebased. @clayborg please take a look at my comment about the timeout setting and let me know if you consider this a blocker or have any other ideas. https://github.com/llvm/llvm-project/pull/84854 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
https://github.com/clayborg approved this pull request. LGTM. I commented on the timeout, but we can implement a setting later if needed. We can always have the ProgressManager class start with 100ms, and then have its timeout updated if/when the settings get modified later. Since there is one progress manager per process, we can just make the setting `global`. https://github.com/llvm/llvm-project/pull/84854 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Implement coalescing of disjoint progress events (PR #84854)
https://github.com/JDevlieghere closed https://github.com/llvm/llvm-project/pull/84854 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
