clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.
================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:89
+ m_start_event_timestamp >=
+ std::chrono::seconds(3);
+}
----------------
The patch description says 5 seconds, here it is 3 seconds, and I would prefer
maybe 1 second.
================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:92
+
+bool TaskProgressEventQueue::SetUpdate(const ProgressEvent &event) {
+ if (event.GetEventType() == progressEnd)
----------------
I would just name this "TaskProgressEventQueue::Update(...)"
================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:93-94
+bool TaskProgressEventQueue::SetUpdate(const ProgressEvent &event) {
+ if (event.GetEventType() == progressEnd)
+ m_finished = true;
+
----------------
If this is an progressEnd event, we could track if this event did report any
progressStart or progressUpdate events and just send the update right away
here? That would require that the report_callback is stored during construction
though. It would be easy if TaskProgressEventQueue just got a reference to the
ProgressEventFilterQueue since it owns these TaskProgressEventQueue. Then we
could extract the callback from it.
================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:113
+ if (m_waiting) {
+ report_callback(m_start_event);
+ m_waiting = false;
----------------
Should we check m_last_update_event here and make sure it isn't a progressEnd
event? It is is, we will notify both start and end right away? Edge case, but
just wanted to check.
================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:124
+ : m_report_callback(report_callback) {
+ m_aux_reporter_thread_finished = false;
+ m_aux_reporter_thread = std::thread([&] {
----------------
This might be better named "m_aux_reporter_thread_should_exit". This variable
sounds like the thread itself will modify the value when it finishes, but this
is a "I want to tell the thread when it is no longer needed when I want it to
exit.
================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:127
+ while (!m_aux_reporter_thread_finished) {
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ ReportWaitingEvents();
----------------
If we are waiting a number of seconds for each progress, not sure we need to
sleep for only 100ms. Shouldn't we sleep for a bit longer time?
================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:151
+
+ while (!m_event_ids.empty()) {
+ uint64_t progress_id = m_event_ids.front();
----------------
If I am reading this loop correctly, we are going to only notify the progress
whose id is m_event_ids.front()? So if we have 10 progress start events we will
only report the first one? Can VS Code only handle one progress at a time? If
not we need to iterate over all m_event_ids and report any possible progress on
all events that have new status as many different threads could be progressing
on indexing DWARF.
================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:157
+ else if (TryReport(it->second))
+ m_event_ids.pop();
+ else
----------------
ProgressEventFilterQueue::TryReport() returns true if the event was reported
even if it didn't finish. Do we always want to pop from m_event_ids even if the
task didn't finish? Is this loop only to catch reporting the start event?
================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:176-177
+ TaskProgressEventQueue &task = it->second;
+ if (task.SetUpdate(event))
+ TryReport(task);
}
----------------
Can't "SetUpdate" just report things if needed? This could be just:
```
it->second.SetUpdate(event);
```
================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:44
uint64_t m_progress_id;
- const char *m_message;
ProgressEventType m_event_type;
----------------
Yikes this must have caused some serious bugs in the reporting mechanism as the
old string would have been freed!!
================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:57
+/// It controls when the event should start being reported to the IDE.
+class TaskProgressEventQueue {
+public:
----------------
TaskProgressEventQueue might be better named "ProgressEventManager"? It doesn't
have a queue, but it does manage event deliveries
================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:61-64
+ /// \return
+ /// \b true if enough seconds have passed since the start event,
+ /// which means the events should start being reported.
+ bool ShouldReport() const;
----------------
make private?
================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:86
+ llvm::Optional<ProgressEvent> m_last_update_event;
+ std::chrono::duration<double> m_start_event_timestamp;
+ bool m_waiting;
----------------
Should be just be storing this in the ProgressEvent? That way we can accurately
track the timestamp for any packets we deliver, like when we deliver one from
m_last_update_event
================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:87
+ std::chrono::duration<double> m_start_event_timestamp;
+ bool m_waiting;
+ bool m_finished;
----------------
After reading the code, this might be more clear if it were named
"m_reported_start_event".
================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:94
+///
+/// We don't short lived events to be sent to the IDE, as they are rendered
+/// in the UI and end up spamming the DAP connection. The only events
----------------
"We don't report"?
================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:118
+
+ std::function<void(ProgressEvent)> m_report_callback;
+ std::map<uint64_t, TaskProgressEventQueue> m_tasks;
----------------
Curious, when we report events, I remember adding a timestamp to the progress
event packet. If we report events later after the timeout that filters quick
progress packets, do we send the original timestamp from when the progress was
reported? We should make sure we do because we want to be able to use this
timing information from the packet log to deduce performance issues.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101128/new/
https://reviews.llvm.org/D101128
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
