[Lldb-commits] [PATCH] D152364: [lldb] Rate limit progress reports -- different approach [WIP-ish]

Fri Jun 9 01:00:20 PDT 2023

labath added a comment.

In D152364#4406589 <https://reviews.llvm.org/D152364#4406589>, @JDevlieghere wrote:

> Continuing some of the discussion from D150805 <https://reviews.llvm.org/D150805> here as it mostly relates to this patch:
>
> In D150805#4402906 <https://reviews.llvm.org/D150805#4402906>, @labath wrote:
>
>> I agree that we should have a rate limiting mechanism very close to the source, to avoid wasting work for events that aren't going to be used.
>
> I'm not sure I (fully) agree with this statement. I may have misunderstood the motivation for D150805 <https://reviews.llvm.org/D150805>, but my understanding was that it was the consumption of the progress events that was too costly, not the generation. Of course this will indirectly make that problem better because of the rate limiting, but as I said in the original review, that still seems like something the consumer should have control over.
>
> I would have assumed that reporting the progress wouldn't be that expensive. Based on the description of this patch, it seems like it's non-trivial, but also not a dominating factor by any means.

That is correct. It is definitely not as slow as I expected it to be at first.

>> This is particularly important for debug info parsing, where we have multiple threads working in parallel and taking a lock even just to check whether we should report something could be a choke point.
>
> Fair enough, but this could be achieved without the rate limiting: the reporting could be an async operation with the thread reporting every event.

That's a bit tricky. If you want to guarantee you don't lose any events *and* also not block the "sending" thread , you have to have some sort of a queuing mechanism -- which means you're essentially reimplementing a listener/broadcaster pattern. I'm sure that could be implemented more efficiently that what our current classes do, but I don't think that would be time well spent. And we'd still be generating a lot of events that noone is going to see.

In D152364#4406927 <https://reviews.llvm.org/D152364#4406927>, @JDevlieghere wrote:

> While generality is part of why I favor doing the rate limiting in the listener, it also means that the listener can decide the rate. For example, VSCode could decide they don't need rate limiting (as is the case today) while the default event handler in LLDB could make a different decision (for example based on whether you're in a fast TTY).

The idea seems nice in principle, but I think the implementation would be somewhat messy. For "normal" events, the goal usually is to send them out as quickly as possible, but in this case we actually want to do the opposite -- force a delay before the receipt (or sending) of a specific event. And as the same listener will be receiving multiple kinds of events, be need this to be configurable on a per-event basis. If I was going down this path, I guess I'd do it by adding some kind of a delay/frequency argument to `Listener::StartListeningForEvents`  function. The listener would remember the requested frequency for a specific type of events as well as the last time that this kind of an event was received, and then the broadcaster would avoid enqueuing these events (and waking up the listener) if the events come faster than that.

Doable, just slightly complicated. We'd also need to figure out what we do with the existing filtering mechanism -- looking at the code, I see that we already have the ability to send "unique" events (`Broadcaster::BroadcastEventIfUnique`). This is a form of rate-limiting, so I think it'd make sense to merge these. However:

- this behavior is controlled on the broadcaster side
- this actually keeps the first duplicated event, whereas we'd most likely prefer to keep the last one

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152364