[LLDB][Progress-On-Dap] Have indeterminate progress actually send events.

[Jlalond]

Recently, I got a report how a user 'hung', and come to find out it's actually because DAP is checking percentage, and on non-deterministic events, we will effectively never send a progress event to DAP. Here we short circuit and don't view percentages for DAP messages and solely depend on the DAP 250ms throttle, which is probably still too aggressive, but better than no updates.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jacob Lalonde (Jlalond)

Changes

Recently, I got a report how a user 'hung', and come to find out it's actually because DAP is checking percentage, and on non-deterministic events, we will effectively never send a progress event to DAP. Here we short circuit and don't view percentages for DAP messages and solely depend on the DAP 250ms throttle, which is probably still too aggressive, but better than no updates.

---

Full diff: https://github.com/llvm/llvm-project/pull/140162.diff

3 Files Affected:

• (modified) lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py (+1-1)
• (modified) lldb/tools/lldb-dap/ProgressEvent.cpp (+7-4)
• (modified) lldb/tools/lldb-dap/ProgressEvent.h (+1-1)

diff --git a/lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py b/lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py
index fee63655de0da..c87d2afe36821 100755
--- a/lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py
+++ b/lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py
@@ -81,7 +81,7 @@ def test(self):
 
         self.verify_progress_events(
             expected_title="Progress tester: Initial Indeterminate Detail",
-            expected_message="Step 1",
+            expected_message="Step 2",
             only_verify_first_update=True,
         )
 
diff --git a/lldb/tools/lldb-dap/ProgressEvent.cpp b/lldb/tools/lldb-dap/ProgressEvent.cpp
index 6a4978c055e51..b6b62efb5f33c 100644
--- a/lldb/tools/lldb-dap/ProgressEvent.cpp
+++ b/lldb/tools/lldb-dap/ProgressEvent.cpp
@@ -77,16 +77,19 @@ ProgressEvent::Create(uint64_t progress_id, std::optional<StringRef> message,
   if (event.GetEventType() == progressStart && event.GetEventName().empty())
     return std::nullopt;
 
-  if (prev_event && prev_event->EqualsForIDE(event))
+  if (prev_event && prev_event->EqualsForIDE(event, total))
     return std::nullopt;
 
   return event;
 }
 
-bool ProgressEvent::EqualsForIDE(const ProgressEvent &other) const {
+bool ProgressEvent::EqualsForIDE(const ProgressEvent &other, uint64_t total) const {
   return m_progress_id == other.m_progress_id &&
-         m_event_type == other.m_event_type &&
-         m_percentage == other.m_percentage;
+         m_event_type == other.m_event_type && 
+         // If we check the percentage of a non-deterministic event
+         // we will basically never send the event, because N+1/Uint64_max
+         // will always be an infinitesimally small change.
+         (total != UINT64_MAX && m_percentage == other.m_percentage);
 }
 
 ProgressEventType ProgressEvent::GetEventType() const { return m_event_type; }
diff --git a/lldb/tools/lldb-dap/ProgressEvent.h b/lldb/tools/lldb-dap/ProgressEvent.h
index d1b9b9dd887cd..ab3487c1dbc3d 100644
--- a/lldb/tools/lldb-dap/ProgressEvent.h
+++ b/lldb/tools/lldb-dap/ProgressEvent.h
@@ -54,7 +54,7 @@ class ProgressEvent {
   /// \return
   ///       \b true if two event messages would result in the same event for the
   ///       IDE, e.g. same rounded percentage.
-  bool EqualsForIDE(const ProgressEvent &other) const;
+  bool EqualsForIDE(const ProgressEvent &other, uint64_t total) const;
 
   llvm::StringRef GetEventName() const;
 

[Jlalond]

@JDevlieghere & @ashgti, thoughts on also making it 500 ms between non-deterministic progress events, or do we think 250 is the best tradeoff of performance to responsiveness?

[ashgti]

Isn't m_percentage a nullopt when the total is UINT64_MAX? Or is there a logic issue in lines 35-65?

[Jlalond]

You're correct, but then we'd be comparing if null_opt == null_opt, and no further updates would be sent. I overlooked this when adding the total, we should short circuit and ignore the percentage if percentage is a null opt.

[ashgti]

But won't they have different event types if its changed? For a non-determinitic progress event, it should only go from m_event_type == progressStart to m_event_type == progressEnd, right? If we have 2 starts or 2 ends that feels like the logic got mixed up somewhere else, right?

[Jlalond]

First, let me apologize for being overly brief in my explanation. You're correct @ashgti, but now imagine all the updates.

Let's say we're indexing dwarf, you'll get the start, the first update and then the end.

As an extreme hypothetical, if this takes 30 minutes you will have 0 progress actually sent to the progress bar in VSCode resulting in what appears to be a hang of LLDB.

So, for non-deterministic events we want to update for every time window. DAP limits to 250 ms, which I think is fine. I might need to change the equals check now that I spell this out but I hope this explains the intent!

[ashgti]

I think that should be fine though, right? Unless the message or progress % changes then we shouldn't need to send an update (see https://microsoft.github.io/debug-adapter-protocol/specification#Events_ProgressUpdate). If this is to update the message, should we add m_message to the Equals check?

[Jlalond]

should we add m_message to the Equals check?

Correct, I was using my progress tester which I realized every event has it's own unique message. But it is optional. Let me fix that...

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[JDevlieghere]

LGTM if @ashgti is happy.

[ashgti]

Looks good to me!