Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 19, 2025
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0673dc5
Have interderminate events actually broadcast to dap
Jlalond May 15, 2025
dabda3b
Format, add regex to test, and check if progress is nullopt instead o…
Jlalond May 15, 2025
5d0bd7d
Switch to comparing messages, not percentage
Jlalond May 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)

Expand Down
11 changes: 7 additions & 4 deletions lldb/tools/lldb-dap/ProgressEvent.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

// 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; }
Expand Down
2 changes: 1 addition & 1 deletion lldb/tools/lldb-dap/ProgressEvent.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
Loading