Prime module caches outside of the module list lock for better parallelism

[scott.smith]

Summary

Loading a shared library can require a large amount of work; rather than do that serially for each library, provide a mechanism to do some amount of priming before hand.

[scott.smith]

Here's the controversial patch. It has been brought up that the intent is to not load symbols before they are needed, but at least in my experience this patch has no effect on performance when running:
lldb -b -o run /path/to/myprogram
where myprogram has main(){return 0;} and links in a whole lot of libraries. My platform is Ubuntu 14.04; are there other systems where symbol processing is actually delayed? Maybe on those systems PrimeCaches() can be a no-op to not impact performance?

This patch requires D32597 in order to actually improve performance, but does not require it for correctness.

[jingham]

Adding Greg as a Reviewer.

[clayborg]

Making an empty main program and saying I see no difference is not enough testing to enable this. I also don't see the benefit of this path. When LLDB is used for symbolication, it might never actually load any debug info or symbols from modules that are added to a target. Also if there is any DWARF that doesn't have the Apple accelerator tables then the DWARF manually indexes itself by spawning threads that will index a compile unit per thread using a thread pool.. So now if we load all symbols at once, using NUM_CORES threads, and then each DWARF file spins off NUM_CORES threads to index the DWARF we can probably bring the machine to the brink? If this feature does go it, we should add a setting that is disabled by default, but if enabled, uses this new functionality. That way we can get it in, ask people to opt into it and later possibly change the default if all goes well. That is my two cents. I still vote for do as little as possible and only do it when needed. But I can see the desire to do things as you coded them. So I think it should be an option that is off by default, but users can set it in their ~/.lldbinit file.

[jingham]

Instead of having the cache priming be determined by platform or something like that, it would be better to add a setting (on the target level seems right) that controls this. I think you are right that in most common usage, there's going to be an unrestricted query that will end up looking through all the symbols, and in that case, having ingested them in parallel would be a win. So the correct default would be to do this eagerly.

The control is more for people who are writing special purpose tools than for ordinary users. For instance, when Xcode runs its "Test" action, by default it uses lldb to run the test binaries. That great because it means that if a test crashes, you'll be right in the debugger and can analyze the failure. But most of the time the tests never actually stop in the debugger. So reading in a whole raft of symbol information in that use case is pretty pointless. They would be a good candidate to use this setting.

Note, this will make the lazy loading symbol path the less common one, so we'll have to be careful to add some tests to make sure that mode still works.

[scott.smith]

In D32598#739779, @clayborg wrote:

Making an empty main program and saying I see no difference is not enough testing to enable this.

It's not quite an empty main program; it links in 40+ shared libraries with 2M+ symbols. The point of having main() just return 0 is so I could compare the performance of "b main; run" vs "run" without taking program execution into account, but just the amount of work lldb does.

But as it turns out, I still managed to screw that up. I'm now seeing a difference:

WITHOUT CHANGE:
lldb -b -o 'b main' -o 'run' xyz:
real 0m15.248s
user 0m24.632s
sys 0m3.839s
lldb -b -o 'run' xyz:
real 0m11.540s
user 0m9.692s
sys 0m0.892s

WITH CHANGE:
real 0m9.747s
user 0m29.507s
sys 0m10.531s

(timings are reasonably representative, but they do change a bit run to run)

I also don't see the benefit of this path.

It improves lldb startup time significantly. Our use cases seem centered around using symbols, so for us, delaying loading of symbols does not help, it just moves when we wait to another point.

When LLDB is used for symbolication, it might never actually load any debug info or symbols from modules that are added to a target. Also if there is any DWARF that doesn't have the Apple accelerator tables then the DWARF manually indexes itself by spawning threads that will index a compile unit per thread using a thread pool.. So now if we load all symbols at once, using NUM_CORES threads, and then each DWARF file spins off NUM_CORES threads to index the DWARF we can probably bring the machine to the brink?

No, because it uses a global TaskPool, so regardless of how many users there are, there will only be NUM_CORES threads. Note my comments in D32597 about changing TaskPool so it will be file-local instead of global, so TaskPool workers can spawn other TaskPool work.

[scott.smith]

In D32598#739804, @jingham wrote:

Instead of having the cache priming be determined by platform or something like that, it would be better to add a setting (on the target level seems right) that controls this. I think you are right that in most common usage, there's going to be an unrestricted query that will end up looking through all the symbols, and in that case, having ingested them in parallel would be a win. So the correct default would be to do this eagerly.

Define "a setting." A method on class Target? A parameter when constructing class Target? Or something via the command line, or lldbinit?

[jingham]

Ah, sorry. I was talking about an lldb command line setting, which you access through the "setting set/get" command. Those are actually implemented by adding a property on the class in question. Look in Target.cpp for the TargetProperties class. You'll just want to add another property there.

[scott.smith]

Added setting. Verified that:
settings set target.cache-priming off
works as expected.

[jingham]

This is picky but can you call it "symbol-cache-priming". The help text explains it, but this is a very specific cache so we should scope it in case we have some other one later on.

And we really need to test that both modes continue to work. If we're always fetching symbols up-front by default, then it will be easy for triggering lazy loading to break. One way to do that would to add another mode of the testsuite (like the dwarf vrs. dSYM vrs. dso vrs. gmodules.) But at some point continually multiplying runs of the whole testsuite is going to make it so time consuming people will stop running all the options.

In this case, it's probably sufficient to add a test that runs to a shared-library scoped breakpoint, and performs some operations that require symbols from other shared-libraries and makes sure they succeed. Then do that with the setting on and off.

[clayborg]

Would "preload-symbols" be a bit more clear and concise?

[jingham]

Yes, I like that better too.

[scott.smith]

1. Rename to preload-symbols / PreloadSymbols()
2. Modify an existing test to run with and without symbol preloading.

[scott.smith]

Fix default param to setup_common so that we actually test both paths.

[jingham]

That test is good. That tests the lazy lookup of the dwarf type indices. The other thing this changes is symbol reading. Can you also add a similar test that relies on finding a symbol in a shared library we might not have read in? It probably fine to add it to this test case... It's unlikely we'll break lazy symbol reading and not also break lazy Dwarf Index building, but you never know. Other than that, this looks fine to me.

[scott.smith]

Add test to print expression (calls func, hence resolves symbols). Also better parameterize the common test to reduce code duplication.

[jingham]

Looks good. Thanks for working on this!

[clayborg]

Looks good.

[scott.smith]

Can someone commit this for me? Thanks!

[jingham]

Sure. Closed with r301609.

include/lldb/Core/Module.h

public:
   }
 
   void SetRemoteInstallFileSpec(const FileSpec &file) {
     m_remote_install_file = file;
   }
 
   const FileSpec &GetSymbolFileFileSpec() const { return m_symfile_spec; }
 
+  void PreloadSymbols();
+
   void SetSymbolFileFileSpec(const FileSpec &file);
 
   const llvm::sys::TimePoint<> &GetModificationTime() const {
     return m_mod_time;
   }
 
   const llvm::sys::TimePoint<> &GetObjectModificationTime() const {
     return m_object_mod_time;

include/lldb/Symbol/SymbolFile.h

public:
   //  virtual uint32_t        FindTypes (const SymbolContext& sc, const
   //  RegularExpression& regex, bool append, uint32_t max_matches, TypeList&
   //  types) = 0;
   virtual TypeList *GetTypeList();
   virtual size_t GetTypes(lldb_private::SymbolContextScope *sc_scope,
                           uint32_t type_mask,
                           lldb_private::TypeList &type_list) = 0;
 
+  virtual void PreloadSymbols();
+
   virtual lldb_private::TypeSystem *
   GetTypeSystemForLanguage(lldb::LanguageType language);
 
   virtual CompilerDeclContext
   FindNamespace(const SymbolContext &sc, const ConstString &name,
                 const CompilerDeclContext *parent_decl_ctx) {
     return CompilerDeclContext();
   }

include/lldb/Symbol/Symtab.h

typedef enum Visibility {
     eVisibilityAny,
     eVisibilityExtern,
     eVisibilityPrivate
   } Visibility;
 
   Symtab(ObjectFile *objfile);
   ~Symtab();
 
+  void PreloadSymbols();
   void Reserve(size_t count);
   Symbol *Resize(size_t count);
   uint32_t AddSymbol(const Symbol &symbol);
   size_t GetNumSymbols() const;
   void SectionFileAddressesChanged();
   void Dump(Stream *s, Target *target, SortOrder sort_type);
   void Dump(Stream *s, Target *target, std::vector<uint32_t> &indexes) const;
   uint32_t GetIndexForSymbol(const Symbol *symbol) const;

include/lldb/Target/Target.h

public:
   void SetDefaultArchitecture(const ArchSpec &arch);
 
   bool GetMoveToNearestCode() const;
 
   lldb::DynamicValueType GetPreferDynamicValue() const;
 
   bool SetPreferDynamicValue(lldb::DynamicValueType d);
 
+  bool GetPreloadSymbols() const;
+
+  void SetPreloadSymbols(bool b);
+
   bool GetDisableASLR() const;
 
   void SetDisableASLR(bool b);
 
   bool GetDetachOnError() const;
 
   void SetDetachOnError(bool b);
 

packages/Python/lldbsuite/test/lang/c/shared_lib/TestSharedLib.py

 """Test that types defined in shared libraries work correctly."""
 
 from __future__ import print_function
 
 
 import unittest2
 import lldb
 from lldbsuite.test.lldbtest import *
 import lldbsuite.test.lldbutil as lldbutil
 
 
 class SharedLibTestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
-    def test_expr(self):
-        """Test that types work when defined in a shared library and forward-declared in the main executable"""
+    def common_test_expr(self, preload_symbols):
         if "clang" in self.getCompiler() and "3.4" in self.getCompilerVersion():
             self.skipTest(
                 "llvm.org/pr16214 -- clang emits partial DWARF for structures referenced via typedef")
 
         self.build()
-        self.common_setup()
+        self.common_setup(preload_symbols)
 
         # This should display correctly.
         self.expect(
             "expression --show-types -- *my_foo_ptr",
             VARIABLES_DISPLAYED_CORRECTLY,
             substrs=[
                 "(foo)",
                 "(sub_foo)",
                 "other_element = 3"])
 
+        self.expect(
+            "expression GetMeASubFoo(my_foo_ptr)",
+            startstr="(sub_foo *) $")
+
+    def test_expr(self):
+        """Test that types work when defined in a shared library and forward-declared in the main executable"""
+        self.common_test_expr(True)
+
+    def test_expr_no_preload(self):
+        """Test that types work when defined in a shared library and forward-declared in the main executable, but with preloading disabled"""
+        self.common_test_expr(False)
+
     @unittest2.expectedFailure("rdar://problem/10704639")
     def test_frame_variable(self):
         """Test that types work when defined in a shared library and forward-declared in the main executable"""
         self.build()
         self.common_setup()
 
         # This should display correctly.
         self.expect(
             "frame variable --show-types -- *my_foo_ptr",
             VARIABLES_DISPLAYED_CORRECTLY,
             substrs=[
                 "(foo)",
                 "(sub_foo)",
                 "other_element = 3"])
 
     def setUp(self):
         # Call super's setUp().
         TestBase.setUp(self)
         # Find the line number to break inside main().
         self.source = 'main.c'
         self.line = line_number(self.source, '// Set breakpoint 0 here.')
         self.shlib_names = ["foo"]
 
-    def common_setup(self):
+    def common_setup(self, preload_symbols = True):
         # Run in synchronous mode
         self.dbg.SetAsync(False)
 
         # Create a target by the debugger.
         target = self.dbg.CreateTarget("a.out")
         self.assertTrue(target, VALID_TARGET)
 
+        self.runCmd("settings set target.preload-symbols " + str(preload_symbols).lower())
+
         # Break inside the foo function which takes a bar_ptr argument.
         lldbutil.run_break_set_by_file_and_line(
             self, self.source, self.line, num_expected_locations=1, loc_exact=True)
 
         # Register our shared libraries for remote targets so they get
         # automatically uploaded
         environment = self.registerSharedLibrariesWithTarget(
             target, self.shlib_names)

source/Core/Module.cpp

           regex, symbol_type, Symtab::eDebugAny, Symtab::eVisibilityAny,
           symbol_indexes);
       SymbolIndicesToSymbolContextList(symtab, symbol_indexes, sc_list);
     }
   }
   return sc_list.GetSize() - initial_size;
 }
 
+void Module::PreloadSymbols() {
+  std::lock_guard<std::recursive_mutex> guard(m_mutex);
+  SymbolVendor * sym_vendor = GetSymbolVendor();
+  if (!sym_vendor) {
+    return;
+  }
+  // Prime the symbol file first, since it adds symbols to the symbol table.
+  if (SymbolFile *symbol_file = sym_vendor->GetSymbolFile()) {
+    symbol_file->PreloadSymbols();
+  }
+  // Now we can prime the symbol table.
+  if (Symtab * symtab = sym_vendor->GetSymtab()) {
+    symtab->PreloadSymbols();
+  }
+}
+
 void Module::SetSymbolFileFileSpec(const FileSpec &file) {
   if (!file.Exists())
     return;
   if (m_symfile_ap) {
     // Remove any sections in the unified section list that come from the
     // current symbol vendor.
     SectionList *section_list = GetSectionList();
     SymbolFile *symbol_file = m_symfile_ap->GetSymbolFile();

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h

public:
   lldb_private::TypeSystem *
   GetTypeSystemForLanguage(lldb::LanguageType language) override;
 
   lldb_private::CompilerDeclContext FindNamespace(
       const lldb_private::SymbolContext &sc,
       const lldb_private::ConstString &name,
       const lldb_private::CompilerDeclContext *parent_decl_ctx) override;
 
+  void PreloadSymbols() override;
+
   //------------------------------------------------------------------
   // PluginInterface protocol
   //------------------------------------------------------------------
   lldb_private::ConstString GetPluginName() override;
 
   uint32_t GetPluginVersion() override;
 
   const lldb_private::DWARFDataExtractor &get_debug_abbrev_data();

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

           }
         }
       }
     }
   }
   return sc_list.GetSize() - prev_size;
 }
 
+void SymbolFileDWARF::PreloadSymbols() {
+  std::lock_guard<std::recursive_mutex> guard(
+      GetObjectFile()->GetModule()->GetMutex());
+  Index();
+}
+
 void SymbolFileDWARF::Index() {
   if (m_indexed)
     return;
   m_indexed = true;
   Timer scoped_timer(
       LLVM_PRETTY_FUNCTION, "SymbolFileDWARF::Index (%s)",
       GetObjectFile()->GetFileSpec().GetFilename().AsCString("<Unknown>"));
 

source/Symbol/SymbolFile.cpp

 #include "lldb/Symbol/TypeSystem.h"
 #include "lldb/Symbol/VariableList.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/StreamString.h"
 #include "lldb/lldb-private.h"
 
 using namespace lldb_private;
 
+void SymbolFile::PreloadSymbols() {
+  // No-op for most implementations.
+}
+
 SymbolFile *SymbolFile::FindPlugin(ObjectFile *obj_file) {
   std::unique_ptr<SymbolFile> best_symfile_ap;
   if (obj_file != nullptr) {
 
     // We need to test the abilities of this section list. So create what it
     // would
     // be with this new obj_file.
     lldb::ModuleSP module_sp(obj_file->GetModule());

source/Symbol/Symtab.cpp

#endif
     //                if (m_method_to_index.GetValueAtIndex(i, entry.value))
     //                    a.Printf ("%s METHOD\n",
     //                    m_symbols[entry.value].GetMangled().GetName().GetCString());
     //            }
     //        }
   }
 }
 
+void Symtab::PreloadSymbols() {
+  std::lock_guard<std::recursive_mutex> guard(m_mutex);
+  InitNameIndexes();
+}
+
 void Symtab::AppendSymbolNamesToMap(const IndexCollection &indexes,
                                     bool add_demangled, bool add_mangled,
                                     NameToIndexMap &name_to_index_map) const {
   if (add_demangled || add_mangled) {
     Timer scoped_timer(LLVM_PRETTY_FUNCTION, "%s", LLVM_PRETTY_FUNCTION);
     std::lock_guard<std::recursive_mutex> guard(m_mutex);
 
     // Create the name index vector to be able to quickly search by name

source/Target/Target.cpp

             size_t num_found =
                 m_images.FindModules(module_spec_copy, found_modules);
             if (num_found == 1) {
               old_module_sp = found_modules.GetModuleAtIndex(0);
             }
           }
         }
 
+        // Preload symbols outside of any lock, so hopefully we can do this for
+        // each library in parallel.
+        if (GetPreloadSymbols())
+          module_sp->PreloadSymbols();
+
         if (old_module_sp &&
             m_images.GetIndexForModule(old_module_sp.get()) !=
                 LLDB_INVALID_INDEX32) {
           m_images.ReplaceModule(old_module_sp, module_sp);
           Module *old_module_ptr = old_module_sp.get();
           old_module_sp.reset();
           ModuleList::RemoveSharedModuleIfOrphaned(old_module_ptr);
         } else

      "The file/path to be used by the executable program for writing its "
      "standard output."},
     {"error-path", OptionValue::eTypeFileSpec, false, 0, nullptr, nullptr,
      "The file/path to be used by the executable program for writing its "
      "standard error."},
     {"detach-on-error", OptionValue::eTypeBoolean, false, true, nullptr,
      nullptr, "debugserver will detach (rather than killing) a process if it "
               "loses connection with lldb."},
+    {"preload-symbols", OptionValue::eTypeBoolean, false, true, nullptr, nullptr,
+     "Enable loading of symbol tables before they are needed."},
     {"disable-aslr", OptionValue::eTypeBoolean, false, true, nullptr, nullptr,
      "Disable Address Space Layout Randomization (ASLR)"},
     {"disable-stdio", OptionValue::eTypeBoolean, false, false, nullptr, nullptr,
      "Disable stdin/stdout for process (e.g. for a GUI application)"},
     {"inline-breakpoint-strategy", OptionValue::eTypeEnum, false,
      eInlineBreakpointsAlways, nullptr, g_inline_breakpoint_enums,
      "The strategy to use when settings breakpoints by file and line. "
      "Breakpoint locations can end up being inlined by the compiler, so that a "

   ePropertyArg0,
   ePropertyRunArgs,
   ePropertyEnvVars,
   ePropertyInheritEnv,
   ePropertyInputPath,
   ePropertyOutputPath,
   ePropertyErrorPath,
   ePropertyDetachOnError,
+  ePropertyPreloadSymbols,
   ePropertyDisableASLR,
   ePropertyDisableSTDIO,
   ePropertyInlineStrategy,
   ePropertyDisassemblyFlavor,
   ePropertyUseHexImmediates,
   ePropertyHexImmediateStyle,
   ePropertyUseFastStepping,
   ePropertyLoadScriptFromSymbolFile,

           nullptr, idx, g_properties[idx].default_uint_value);
 }
 
 bool TargetProperties::SetPreferDynamicValue(lldb::DynamicValueType d) {
   const uint32_t idx = ePropertyPreferDynamic;
   return m_collection_sp->SetPropertyAtIndexAsEnumeration(nullptr, idx, d);
 }
 
+bool TargetProperties::GetPreloadSymbols() const {
+  const uint32_t idx = ePropertyPreloadSymbols;
+  return m_collection_sp->GetPropertyAtIndexAsBoolean(
+      nullptr, idx, g_properties[idx].default_uint_value != 0);
+}
+
+void TargetProperties::SetPreloadSymbols(bool b) {
+  const uint32_t idx = ePropertyPreloadSymbols;
+  m_collection_sp->SetPropertyAtIndexAsBoolean(nullptr, idx, b);
+}
+
 bool TargetProperties::GetDisableASLR() const {
   const uint32_t idx = ePropertyDisableASLR;
   return m_collection_sp->GetPropertyAtIndexAsBoolean(
       nullptr, idx, g_properties[idx].default_uint_value != 0);
 }
 
 void TargetProperties::SetDisableASLR(bool b) {
   const uint32_t idx = ePropertyDisableASLR;