[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:35
+
+  if (m_event_type == progressStart)
+    m_message = message;
----------------



================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:102
+    : m_start_event(start_event),
+      m_start_event_timestamp(
+          std::chrono::system_clock::now().time_since_epoch()),
----------------
remove m_start_event_timestamp as it is no longer needed since ProgressEvent has a timestamp now.


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:106
+
+bool ProgressEventManager::TryReport() {
+  // We only report if we have already started reporting or if enough time has
----------------
rename to "ReportIfNeeded()"?


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:109-118
+  if (!m_start_event.Reported() &&
+      std::chrono::system_clock::now().time_since_epoch() -
+              m_start_event.GetTimestamp() <
+          std::chrono::seconds(1))
+    return false;
+
+  if (!m_start_event.Reported() && !Finished()) {
----------------



================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:116-117
+  if (!m_start_event.Reported() && !Finished()) {
+    m_report_callback(m_start_event);
+    m_start_event.MarkAsReported();
+  }
----------------
We have many places in the code that are reporting progress and then having to manually mark the event as reported. We should make a ProgressEvent::Report() function that looks something like:
```
void ProgressEvent::Report(ProgressEventReportCallback report_callback) {
  if (!m_reported) {
    m_reported = true;
    report_callback(*this);
  }
}
```






================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:120-121
+  if (m_last_update_event && !m_last_update_event->Reported()) {
+    m_report_callback(*m_last_update_event);
+    m_last_update_event->MarkAsReported();
+  }
----------------



================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:146
+    while (!m_aux_reporter_thread_should_exit) {
+      std::this_thread::sleep_for(std::chrono::seconds(2));
+      ReportWaitingEvents();
----------------
We have a 1 second timeout before events should be reported, so we should probably sleep a shorter amount of time than 2 seconds as if the events come in such that a progress start event is queued right after this loop sleeps for 2 seconds, it can now be three seconds before an event is reported. How about 250 ms? That way the most we will wait before reporting a start event is 1.25 seconds. Also since we expect the thread to exit in ~ProgressEventFilterQueue, we don't want to have to wait 2 seconds during shut down. 


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:157
+
+void ProgressEventFilterQueue::ReportWaitingEvents() {
+  std::lock_guard<std::mutex> locker(m_reporter_mutex);
----------------
Should this be named "ReportStartEvents()"? Is that the only thing this is doing here?


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:165
+    else if (event_manager->TryReport())
+      m_sorted_event_managers
+          .pop(); // we remove it from the queue as it started reporting
----------------
If we are using m_sorted_event_managers only for reporting start events, we should rename to "m_unreported_start_events"


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:170
+    else
+      break; // If we couldn't report it, then the next event in the queue won't
+             // be able as well, as it came later.
----------------
Ok so this class is a start event reporter then?


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:132
+  /// Thread used to invoke \a ReportWaitingEvents periodically.
+  std::thread m_aux_reporter_thread;
+  bool m_aux_reporter_thread_should_exit;
----------------
m_thread should suffice here.


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:133
+  std::thread m_aux_reporter_thread;
+  bool m_aux_reporter_thread_should_exit;
+  /// Mutex that prevents running \a Push and \a ReportWaitingEvents
----------------
m_thread_should_exit should suffice.


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:136
+  /// simultaneously, as both read and modify the same underlying objects.
+  std::mutex m_reporter_mutex;
 };
----------------
m_mutex is fine unless we get more than one mutex in this class.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101128/new/

https://reviews.llvm.org/D101128