Huge performance improvements when one breakpoint contains many locations.

325,000 breakpoints for running "breakpoint set --func-regex ." on lldb itself (after hitting a breakpoint at main so that LLDB.framework is loaded) used to take up to an hour to set, now we are down under a minute. With warm file caches, we are at 40 seconds, and that is with setting 325,000 breakpoint through the GDB remote API. Linux and the native debuggers might be faster. I haven't timed what how much is debug info parsing and how much is the protocol traffic to/from GDB remote.

That there were many performance issues. Most of them were due to storing breakpoints in the wrong data structures, or using the wrong iterators to traverse the lists, traversing the lists in inefficient ways, and not optimizing certain function name lookups/symbol merges correctly.

Debugging after that is also now very efficient. There were issues with replacing the breakpoint opcodes in memory that was read, and those routines were also fixed.



git-svn-id: https://llvm.org/svn/llvm-project/lldb/trunk@183820 91177308-0d34-0410-b5e6-96231b3b80d8

Author: Greg Clayton

include/lldb/Breakpoint/BreakpointSiteList.h

@@ -16,6 +16,7 @@
 // Other libraries and framework includes
 // Project includes
 #include "lldb/Breakpoint/BreakpointSite.h"
+#include "lldb/Host/Mutex.h"
 
 namespace lldb_private {
 
@@ -130,31 +131,8 @@ friend class Process;
     bool
     BreakpointSiteContainsBreakpoint (lldb::break_id_t bp_site_id, lldb::break_id_t bp_id);
 
-    //------------------------------------------------------------------
-    /// Returns a shared pointer to the breakpoint site with index \a i.
-    ///
-    /// @param[in] i
-    ///   The breakpoint site index to seek for.
-    ///
-    /// @result
-    ///   A shared pointer to the breakpoint site.  May contain a NULL pointer if the
-    ///   breakpoint doesn't exist.
-    //------------------------------------------------------------------
-    lldb::BreakpointSiteSP
-    GetByIndex (uint32_t i);
-
-    //------------------------------------------------------------------
-    /// Returns a shared pointer to the breakpoint site with index \a i - const version.
-    ///
-    /// @param[in] i
-    ///   The breakpoint site index to seek for.
-    ///
-    /// @result
-    ///   A shared pointer to the breakpoint site.  May contain a NULL pointer if the
-    ///   breakpoint doesn't exist.
-    //------------------------------------------------------------------
-    const lldb::BreakpointSiteSP
-    GetByIndex (uint32_t i) const;
+    void
+    ForEach (std::function <void(BreakpointSite *)> const &callback);
 
     //------------------------------------------------------------------
     /// Removes the breakpoint site given by \b breakID from this list.
@@ -179,9 +157,6 @@ friend class Process;
     //------------------------------------------------------------------
     bool
     RemoveByAddress (lldb::addr_t addr);
-
-    void
-    SetEnabledForAll(const bool enable, const lldb::break_id_t except_id = LLDB_INVALID_BREAK_ID);
 
     bool
     FindInRange (lldb::addr_t lower_bound, lldb::addr_t upper_bound, BreakpointSiteList &bp_site_list) const;
@@ -211,8 +186,18 @@ friend class Process;
     ///   The number of elements.
     //------------------------------------------------------------------
     size_t
-    GetSize() const { return m_bp_site_list.size(); }
+    GetSize() const
+    {
+        Mutex::Locker locker(m_mutex);
+        return m_bp_site_list.size();
+    }
 
+    bool
+    IsEmpty() const
+    {
+        Mutex::Locker locker(m_mutex);
+        return m_bp_site_list.empty();
+    }
 protected:
     typedef std::map<lldb::addr_t, lldb::BreakpointSiteSP> collection;
 
@@ -222,13 +207,7 @@ friend class Process;
     collection::const_iterator
     GetIDConstIterator(lldb::break_id_t breakID) const;
 
-    // This function exposes the m_bp_site_list.  I use the in Process because there
-    // are places there where you want to iterate over the list, and it is less efficient
-    // to do it by index.  FIXME: Find a better way to do this.
-
-    const collection *
-    GetMap ();
-
+    mutable Mutex m_mutex;
     collection m_bp_site_list;  // The breakpoint site list.
 };
 

include/lldb/Symbol/SymbolContext.h

@@ -442,6 +442,11 @@ class SymbolContextList
     AppendIfUnique (const SymbolContext& sc, 
                     bool merge_symbol_into_function);
 
+    bool
+    MergeSymbolContextIntoFunctionContext (const SymbolContext& symbol_sc,
+                                           uint32_t start_idx = 0,
+                                           uint32_t stop_idx = UINT32_MAX);
+
     uint32_t
     AppendIfUnique (const SymbolContextList& sc_list, 
                     bool merge_symbol_into_function);
@@ -485,6 +490,30 @@ class SymbolContextList
     bool
     GetContextAtIndex(size_t idx, SymbolContext& sc) const;
 
+    //------------------------------------------------------------------
+    /// Direct reference accessor for a symbol context at index \a idx.
+    ///
+    /// The index \a idx must be a valid index, no error checking will
+    /// be done to ensure that it is valid.
+    ///
+    /// @param[in] idx
+    ///     The zero based index into the symbol context list.
+    ///
+    /// @return
+    ///     A const reference to the symbol context to fill in.
+    //------------------------------------------------------------------
+    SymbolContext&
+    operator [] (size_t idx)
+    {
+        return m_symbol_contexts[idx];
+    }
+    
+    const SymbolContext&
+    operator [] (size_t idx) const
+    {
+        return m_symbol_contexts[idx];
+    }
+
     //------------------------------------------------------------------
     /// Get accessor for the last symbol context in the list.
     ///

source/Breakpoint/BreakpointResolverName.cpp

@@ -188,7 +188,7 @@ BreakpointResolverName::SearchCallback
 )
 {
     SymbolContextList func_list;
-    SymbolContextList sym_list;
+    //SymbolContextList sym_list;
 
     uint32_t i;
     bool new_location;
@@ -203,11 +203,10 @@ BreakpointResolverName::SearchCallback
             log->Warning ("Class/method function specification not supported yet.\n");
         return Searcher::eCallbackReturnStop;
     }
-    
-    const bool include_symbols = false;
+    bool filter_by_cu = (filter.GetFilterRequiredItems() & eSymbolContextCompUnit) != 0;
+    const bool include_symbols = filter_by_cu == false;
     const bool include_inlines = true;
     const bool append = true;
-    bool filter_by_cu = (filter.GetFilterRequiredItems() & eSymbolContextCompUnit) != 0;
 
     switch (m_match_type)
     {
@@ -228,26 +227,14 @@ BreakpointResolverName::SearchCallback
 
                     if (start_func_idx < end_func_idx)
                         lookup.Prune (func_list, start_func_idx);
-                    // If the search filter specifies a Compilation Unit, then we don't need to bother to look in plain
-                    // symbols, since all the ones from a set compilation unit will have been found above already.
-                    else if (!filter_by_cu)
-                    {
-                        const size_t start_symbol_idx = sym_list.GetSize();
-                        context.module_sp->FindFunctionSymbols (lookup.lookup_name, lookup.name_type_mask, sym_list);
-                        const size_t end_symbol_idx = sym_list.GetSize();
-                        if (start_symbol_idx < end_symbol_idx)
-                            lookup.Prune (func_list, start_symbol_idx);
-                    }
                 }
             }
             break;
         case Breakpoint::Regexp:
             if (context.module_sp)
             {
-                if (!filter_by_cu)
-                    context.module_sp->FindSymbolsMatchingRegExAndType (m_regex, eSymbolTypeCode, sym_list);
-                context.module_sp->FindFunctions (m_regex, 
-                                                  include_symbols,
+                context.module_sp->FindFunctions (m_regex,
+                                                  !filter_by_cu, // include symbols only if we aren't filterning by CU
                                                   include_inlines, 
                                                   append, 
                                                   func_list);
@@ -281,33 +268,6 @@ BreakpointResolverName::SearchCallback
     SymbolContext sc;
     if (func_list.GetSize())
     {
-        for (i = 0; i < func_list.GetSize(); i++)
-        {
-            if (func_list.GetContextAtIndex(i, sc) == false)
-                continue;
-            
-            if (sc.function == NULL)
-                continue;
-            uint32_t j = 0;
-            while (j < sym_list.GetSize())
-            {
-                SymbolContext symbol_sc;
-                if (sym_list.GetContextAtIndex(j, symbol_sc))
-                {
-                    if (symbol_sc.symbol && symbol_sc.symbol->ValueIsAddress())
-                    {
-                        if (sc.function->GetAddressRange().GetBaseAddress() == symbol_sc.symbol->GetAddress())
-                        {
-                            sym_list.RemoveContextAtIndex(j);
-                            continue;   // Don't increment j
-                        }
-                    }
-                }
-                
-                j++;
-            }
-        }
-        
         for (i = 0; i < func_list.GetSize(); i++)
         {
             if (func_list.GetContextAtIndex(i, sc))
@@ -320,14 +280,21 @@ BreakpointResolverName::SearchCallback
                 else if (sc.function)
                 {
                     break_addr = sc.function->GetAddressRange().GetBaseAddress();
-                    if (m_skip_prologue)
+                    if (m_skip_prologue && break_addr.IsValid())
                     {
-                        if (break_addr.IsValid())
-                        {
-                            const uint32_t prologue_byte_size = sc.function->GetPrologueByteSize();
-                            if (prologue_byte_size)
-                                break_addr.SetOffset(break_addr.GetOffset() + prologue_byte_size);
-                        }
+                        const uint32_t prologue_byte_size = sc.function->GetPrologueByteSize();
+                        if (prologue_byte_size)
+                            break_addr.SetOffset(break_addr.GetOffset() + prologue_byte_size);
+                    }
+                }
+                else if (sc.symbol)
+                {
+                    break_addr = sc.symbol->GetAddress();
+                    if (m_skip_prologue && break_addr.IsValid())
+                    {
+                        const uint32_t prologue_byte_size = sc.symbol->GetPrologueByteSize();
+                        if (prologue_byte_size)
+                            break_addr.SetOffset(break_addr.GetOffset() + prologue_byte_size);
                     }
                 }
 
@@ -351,35 +318,6 @@ BreakpointResolverName::SearchCallback
         }
     }
 
-    for (i = 0; i < sym_list.GetSize(); i++)
-    {
-        if (sym_list.GetContextAtIndex(i, sc))
-        {
-            if (sc.symbol && sc.symbol->ValueIsAddress())
-            {
-                break_addr = sc.symbol->GetAddress();
-                
-                if (m_skip_prologue)
-                {
-                    const uint32_t prologue_byte_size = sc.symbol->GetPrologueByteSize();
-                    if (prologue_byte_size)
-                        break_addr.SetOffset(break_addr.GetOffset() + prologue_byte_size);
-                }
-                
-                if (filter.AddressPasses(break_addr))
-                {
-                    BreakpointLocationSP bp_loc_sp (m_breakpoint->AddLocation(break_addr, &new_location));
-                    if (bp_loc_sp && new_location && !m_breakpoint->IsInternal())
-                    {
-                        StreamString s;
-                        bp_loc_sp->GetDescription(&s, lldb::eDescriptionLevelVerbose);
-                        if (log) 
-                            log->Printf ("Added location: %s\n", s.GetData());
-                    }
-                }
-            }
-        }
-    }
     return Searcher::eCallbackReturnContinue;
 }