[Lldb-commits] [PATCH] D101128: [lldb-vscode] only report long running progress events

[Greg Clayton via Phabricator via lldb-commits]

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