[Lldb-commits] [PATCH] D150805: Rate limit progress reporting

[Sterling Augustine via Phabricator via lldb-commits]

saugustine added a comment.

In D150805#4397940 <https://reviews.llvm.org/D150805#4397940>, @rupprecht wrote:

> In D150805#4350849 <https://reviews.llvm.org/D150805#4350849>, @JDevlieghere wrote:
>
>> I also like Jordan's rate limiting idea. In my mind that should be a property of the broadcaster. Different tools (e.g. vscode vs the command line) could specify different values when register a listener.
>
> This makes sense: we could augment `lldb_private::Listener` with additional members that keep track of when the last broadcast time was, and if we're rate limiting. Then we could change the implementation of `Listener::GetEvent(lldb::EventSP &event_sp, const Timeout<std::micro> &timeout)` to continuously churn through `m_events`, returning the most recent one by the time the rate limiting window is over, and discarding any intermediate ones in between.
>
> One thing I'm not sure of though is how we'll avoid an unnecessary pause for rate limiting on the last item. This patch avoids that because it checks `data->GetCompleted() != data->GetTotal()` to decide if we should actually rate limit. In the generic case, how does the listener know that an event it returns is the final one, and that it should ignore the rate limiting delay?
>
> I think we could address that by adding a `bool m_important` member to `lldb::Event`, and then it would be up to the broadcaster to set that to true for the last one (or any intermediate ones that are similarly important & should be immediately shown, e.g. warnings/errors). Would that be reasonable?

The later a decision the decision not to update is made, the more work is wasted. Even the fairly simple solution that checked time in a somewhat expensive way (via the misnamed getCurrentTime that also gets memory used) ended up being slower overall. In my opinion, the problem is that a single-die is too small a unit of work to be worth reporting on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150805