[lldb] Add timed callbacks to the MainLoop class #112895
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,27 +7,43 @@ | |
| //===----------------------------------------------------------------------===// | ||
|
|
||
| #include "lldb/Host/MainLoopBase.h" | ||
| #include <chrono> | ||
|
|
||
| using namespace lldb; | ||
| using namespace lldb_private; | ||
|
|
||
| void MainLoopBase::AddCallback(const Callback &callback, TimePoint point) { | ||
| bool interrupt_needed; | ||
| { | ||
| std::lock_guard<std::mutex> lock{m_callback_mutex}; | ||
| // We need to interrupt the main thread if this callback is scheduled to | ||
| // execute at an earlier time than the earliest callback registered so far. | ||
| interrupt_needed = m_callbacks.empty() || point < m_callbacks.top().first; | ||
| m_callbacks.emplace(point, callback); | ||
| } | ||
| if (interrupt_needed) | ||
|
There was a problem hiding this comment. If I understand the logic here, we're checking before emplacement if an interrupt is needed and then once we emplace we Interrupt,
There was a problem hiding this comment.
That's correct.
Empty callbacks mean an infinite timeout (line MainLoopPosix.cpp, line 52). In that case, we definitely need to interrupt the polling thread (so it can go to sleep with a new timeout). In other cases, we only need to interrupt if this operation is scheduled to run before the previous earliest operation.
I think you're misunderstanding something here. The emplace is always fast - it just adds something to the queue and doesn't cause any callbacks to run. The callbacks will always be run on the mainloop thread. The interrupt needs to happen after the insertion to make sure the other thread observes the new callback (via the result of There was a problem hiding this comment.
My concern was purely if we want nanosecond precision, adding into a priority queue (assuming N log N) seemed like it could be slow. On a second look, I think my concern is moot |
||
| Interrupt(); | ||
| } | ||
|
|
||
| void MainLoopBase::ProcessCallbacks() { | ||
| while (true) { | ||
| Callback callback; | ||
| { | ||
| std::lock_guard<std::mutex> lock{m_callback_mutex}; | ||
| if (m_callbacks.empty() || | ||
| std::chrono::steady_clock::now() < m_callbacks.top().first) | ||
| return; | ||
| callback = std::move(m_callbacks.top().second); | ||
| m_callbacks.pop(); | ||
| } | ||
|
|
||
| callback(*this); | ||
| } | ||
| } | ||
|
|
||
| std::optional<MainLoopBase::TimePoint> MainLoopBase::GetNextWakeupTime() { | ||
| std::lock_guard<std::mutex> lock(m_callback_mutex); | ||
|
There was a problem hiding this comment. Should this be a recursive mutex? Is there any situation where invoking a callback will end us back at this codepath? I know it's unlikely but my question is if it's impossible There was a problem hiding this comment. No, the callbacks run with the mutex unlocked. But even if they were running with mutex held, making this a recursive mutex would not help, as the code would likely not work correctly because the outer function would get surprised by the callback collection mutation under it -- even though it has it "locked". We've had several bugs like that over the years, last one being #96750. |
||
| if (m_callbacks.empty()) | ||
| return std::nullopt; | ||
| return m_callbacks.top().first; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are Nanoseconds here overly precise? Will we imply that callbacks will have nano-second level precision in timing out? I think because this is used for timeouts milliseconds is sufficient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not likely to be useful, but we do have system APIs which provide nanosecond level resolution, so why not make it available. Users don't have to specify timeouts with a nanosecond level. They can just say
std::chrono::seconds/*or whatever*/(X), and it will be automatically to nanoseconds.Using a lower precision would actually be somewhat unergonomic chrono conversions which lose precision are not implicit. So something like
steady_clock::now()+seconds(2)would not be implicitly convertible to milliseconds if the clock resolution was more than milliseconds (which it usually is).