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

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:89
+                               m_start_event_timestamp >=
+                           std::chrono::seconds(3);
+}
----------------
clayborg wrote:
> The patch description says 5 seconds, here it is 3 seconds, and I would prefer maybe 1 second.
yes, 1 second is reasonable. I'll test it with a huge binary as well


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:93-94
+bool TaskProgressEventQueue::SetUpdate(const ProgressEvent &event) {
+  if (event.GetEventType() == progressEnd)
+    m_finished = true;
+
----------------
clayborg wrote:
> 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.
good one


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:113
+  if (m_waiting) {
+    report_callback(m_start_event);
+    m_waiting = false;
----------------
clayborg wrote:
> 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.
good catch


================
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([&] {
----------------
clayborg wrote:
> 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.
+1


================
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();
----------------
clayborg wrote:
> 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?
yes, you are right, at least 1 second should be fine.


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:151
+
+  while (!m_event_ids.empty()) {
+    uint64_t progress_id = m_event_ids.front();
----------------
clayborg wrote:
> 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.
not quite right, this will report as many as it can until it finds the first one who can't be reported, which means that that event is not old enough, and therefore the other events are also not old enough


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:157
+    else if (TryReport(it->second))
+      m_event_ids.pop();
+    else
----------------
clayborg wrote:
> 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?
this is only looping through the start events. I'll make this clearer


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:44
   uint64_t m_progress_id;
-  const char *m_message;
   ProgressEventType m_event_type;
----------------
clayborg wrote:
> Yikes this must have caused some serious bugs in the reporting mechanism as the old string would have been freed!!
well, not really as we were dispatching the event right away, but with async events we need to save the data to a string


================
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;
----------------
clayborg wrote:
> 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
makes sense


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:118
+
+  std::function<void(ProgressEvent)> m_report_callback;
+  std::map<uint64_t, TaskProgressEventQueue> m_tasks;
----------------
clayborg wrote:
> 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.
i'll put that back!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101128