[Lldb-commits] Performing ManualDWARFIndex::Index() async during ManualDWARFIndex::Preload()

Sun Feb 7 02:35:19 PST 2021

Hi Pavel,

Thanks for the detailed response and sorry for the late reply. I was very
busy this week and I do this work as part of a side project (so I might
need some time to react).
I hope I will find some time next week to work on it.

We tried 'target.preload-symbols'. Unfortunately, it only led to the lag
spike occurring later but not disappearing completely.
I will do some tests and let you know of the results.

2) You mentioned:
*I might even go for a once_flag/call_once to also get the "runs once"
behavior for free.*
Is this a common pattern/utility in llvm? (This is my first llvm
contribution, so let me know if I should use any pattern/utility)
Or do you mean to create a flag *ManualDWARFIndex *and use it to check at
the beginning of Preload() to avoid creating the async task?

As for the locking. I have seen that the lock is already taken in
*SymbolFileDWARF::PreloadSymbols().
*My concern is that the code after PreloadSymbols() is now (potentially)
executed in parallel to the code in Index().

3) I will also take a look with some tests to check if I can provoke
contention issues.

kind regards,
Lasse

On Sun, Jan 31, 2021 at 2:21 PM Pavel Labath <pavel at labath.sk> wrote:

> On 29/01/2021 16:25, Lasse Folger via lldb-commits wrote:
>  > Hi lldb devs,
>  >
>  > We experienced some issues when debugging large binaries.
>  > The startup time of lldb for large binaries is long (tens of seconds).
>  > One of the reasons is that the /ManualDWARFIndex::Index()/ function is
>  > executed synchronously and thus blocks during startup.
>
> Before I go further, let me ask this: You are aware of the
> 'target.preload-symbols' setting which can disable (delay) the indexing
> at starting, right? I am asking because it may result in a similar
> behavior than what you get with your patch. At the very least, it would
> be interesting to see a comparison of the two scenarios, to get a feel
> for the benefits it offers.
>
> On 29/01/2021 18:23, Lasse Folger via lldb-commits wrote:
> > Thanks Raphael for the pointers, I will take a look.
> >
> > I just noticed I forgot to attach the actual patch I prepared ;)
> > Here it is.
> >
> > On Fri, Jan 29, 2021, 5:02 PM Raphael “Teemperor” Isemann
> > <teemperor at gmail.com <mailto:teemperor at gmail.com>> wrote:
> >
> >     I can't help with any of the requested feedback, but the last time I
> >     looked at a benchmark involving ManualDWARFIndex, the
> >     ManualDWARFIndex::IndexUnit spent about 40% of its runtime just
> >     constructing ConstString. Half of that 40% (so, 20% total time) was
> >     just spend contesting/locking the locks for ConstString's global
> >     string pools. Refactoring the code to not having to reaquire a
> >     contested global lock for every DIE might help with improving
> >     startup time.
>
> I also feel obligated to point out that this code has been optimized and
> re-optimized by a number of people already. So all of the quick wins are
> likely gone already.
>
> I am not saying that its not possible to improve the performance of this
> (in fact, I'm certain it can) -- however, such an effort will probably
> require making fundamental changes to the algorithm, rather than just
> using fancier data structures.
>
> I don't want to sound too discouraging, but I think you ought to know
> what you're getting yourself into.
>
> Good luck! (I mean it)
>
> >>     I would like to have feedback on three parts:
> >>
> >>      1. In /ManualDWARFIndex::Preload() /I capture /this/ of the
> >>         /ManualDWARFIndex/. As far as I could tell this is not a
> >>         problem since the index is not relocated or deleted. However,
> >>         I'm not that familiar with the architecture. Are there cases
> >>         when the object is relocated or deleted before lldb stops?
> While it's very unlikely, I don't think we can guarantee that the Module
> object holding the index is not deleted by the time the async thread
> runs. Such a thing might happen due to events totally beyond our
> control, such as the user calling SBTarget::RemoveModule.
>
> This should be fixable by simply stashing the Module shared pointer.
>
>
> >>      2. I use the /Module /mutex for synchronization because I didn't
> >>         want to introduce a mutex in /ManualDWARFIndex /or
> >>         /DWARFIndex/. Do you think this locking granularity is fine or
> >>         should I create a dedicated mutex for it?
> I think it should work, but a separate synchronization primitive might
> be cleaner. I might even go for a once_flag/call_once to also get the
> "runs once" behavior for free.
>
> >>         Furthermore, I
> >>         looked through the implementation
> >>         of ManualDWARFIndex::/Index()/ and couldn't find any locking
> >>         function in there. However, I might have missed something (or
> >>         some of the underlying implementations change in the future)
> >>         and there is a danger for a deadlock.
> The lack of locking is not surprising because these functions already
> expect that the caller has taken the Module lock.
>
> >>      3. /ManualDWARFIndex::Index()/ creates a thread pool internally.
> >>         This means if multiple calls to/ ManualDWARFIndex::Index()
> >>         /are executed at the same time, there might be more threads
> >>         than cores. This might cause contention and decrease
> >>         efficiency. (Thanks to Andy for pointing this out).
>
> This is an interesting question for me, but I don't really have a good
> answer to it. On one hand, you could say that this is a situation that
> could occur already (though it wasn't always the case -- back when we
> had our own thread pool, it was process-wide and shared between
> different module instances). But OTOH, this does make it much easier for
> this situation to occur...
>
> pl
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20210207/c6575c0f/attachment.html>