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

[Jordan Rupprecht via Phabricator via lldb-commits]

rupprecht added a subscriber: labath.
rupprecht added a comment.

The other tricky part I missed before is this bit in between pulling the event off the listener queue and deciding to show it:

  auto *data = ProgressEventData::GetEventDataFromEvent(event_sp.get());
  if (!data)
    return;
  
  // Do some bookkeeping for the current event, regardless of whether we're
  // going to show the progress.
  const uint64_t id = data->GetID();
  if (m_current_event_id) {
    ...
    if (id != *m_current_event_id)
      return;
    if (data->GetCompleted() == data->GetTotal())
      m_current_event_id.reset();
  } else {
    m_current_event_id = id;
  }

When we pull the event off listener off the queue, we need to do post-processing ourselves to decide if it's even a progress event at all, and if it's meant for us. If we put the listener in charge of rate limiting and returning only the most recent event, it needs to know that the event it's returning is interesting. Otherwise the rate limiting might hide all the interesting events. On top of that, there are events that are *interesting* even if we don't want to show them.

Another option is to provide `Listener::GetRateLimitedEvents` (name TBD) instead. It (potentially) blocks for the rate limiting period and then returns a *list* of all the events that have happened within that time frame. Then we let the caller process it as it pleases and display only the most recent relevant one. It feels a little weird at first but might actually make sense compared to alternatives?

I think it would be nice to abstract the rate limiting somewhere, although I'm not sure anymore if having it directly in the broadcast/listen classes makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150805