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 <pa...@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 > > <teempe...@gmail.com <mailto:teempe...@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 >
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
