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