[Lldb-commits] [PATCH] D97739: Add a progress class that can track and report long running operations that happen in LLDB.

Mon Mar 8 10:07:10 PST 2021

jingham added a comment.

In D97739#2610521 <https://reviews.llvm.org/D97739#2610521>, @clayborg wrote:

> In D97739#2608510 <https://reviews.llvm.org/D97739#2608510>, @friss wrote:
>
>> In D97739#2607961 <https://reviews.llvm.org/D97739#2607961>, @clayborg wrote:
>>
>>> In D97739#2607869 <https://reviews.llvm.org/D97739#2607869>, @jingham wrote:
>>>
>>>> This way of doing progress is going to look odd in anything that uses multiple debuggers.  If I'm reading the code aright, if I have two debuggers, and a target in Debugger A starts doing something that would cause progress traffic, both debuggers will show activity.
>>>
>>> that is true, but it is a global module repository that benefits both debuggers. And I very rarely debug two things at the same time, so most of the time for most people this will be beneficial and shouldn't cause too much confusion.
>>
>> Just one tidbit here. Most users are actually routinely running tens of debuggers at the same time, because tests run in parallel and they have a debugger attached by default. Now if you have a long running operation kick in in your unit tests, you might already have a different kind of issue, but I’d like to avoid a world where the IDE displays spurious and wrong information because of this.
>
> But you wouldn't actually hookup any progress callbacks on any of these debuggers right? You don't make a window for each test, IIRC you are only running it under the debugger so you can report issues by using the debugger. Am I remember this correctly? What happens when a test crashes? If you are running 100 tests in parallel and 10 of them crash, do you open a window for each one that does crash? Or do you manually have to debug the test in order to stop at breakpoints or at a crash?
>
> If we truly need debugger specific task progress, that is a lot more work and we have no great solution. One idea is we could end up having progress items take a SymbolContextScope pointer as an optional parameter that would allow us to grab the module pointer from it and then ask the debugger if any targets contain this module prior to reporting progress. This would be a bit expensive code for a lot of quick progress updates as we would need to iterate over each target and each target's module list and we would still have many system libraries reporting progress to all debuggers when targets contain the same system libraries.
>
> The threading overhead and expense of delivering SBEvents also seems like overkill as threading itself and waiting using a mutex + condition will slow down progress delivery especially if we have a progress that does a bunch of updates. And once we receive the event we will need to make a static function call to extract all progress variables (total, completed, progress_id, baton, etc).

I don't want to push this too hard, either way will work.  But note that one of the big advantages of the event version is that we aren't running arbitrary code fairly deep in the Symbol side of lldb.  At the cost of taking a lock to deliver the event, we get all this code out of the depth of the symbol reader and into a separate reporter thread.

I think the important thing here is that the code that detects the progress event not stall the Symbol processing.  But I don't see the need to be super-performant in reporting the event.  If the progress events are going by so fast that taking a lock to get the event and report on it matters, then the progress was going fast enough that the user probably won't care about it.

Another nice thing about the event version is that it allows you to coalesce reports if they come in too quickly, since you can peek at the event queue when you get the first event, and pull any other pending ones off.  So for instance if you get a "started event" and peeking at the queue find that you also already have the completed event, you could choose not to report progress, or just completed...

> In D97739#2608510 <https://reviews.llvm.org/D97739#2608510>, @friss wrote:
>
>> In D97739#2607961 <https://reviews.llvm.org/D97739#2607961>, @clayborg wrote:
>>
>>> In D97739#2607869 <https://reviews.llvm.org/D97739#2607869>, @jingham wrote:
>>>
>>>> This way of doing progress is going to look odd in anything that uses multiple debuggers.  If I'm reading the code aright, if I have two debuggers, and a target in Debugger A starts doing something that would cause progress traffic, both debuggers will show activity.
>>>
>>> that is true, but it is a global module repository that benefits both debuggers. And I very rarely debug two things at the same time, so most of the time for most people this will be beneficial and shouldn't cause too much confusion.
>>
>> Just one tidbit here. Most users are actually routinely running tens of debuggers at the same time, because tests run in parallel and they have a debugger attached by default. Now if you have a long running operation kick in in your unit tests, you might already have a different kind of issue, but I’d like to avoid a world where the IDE displays spurious and wrong information because of this.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97739