[Lldb-commits] [PATCH] D104067: [lldb] Decouple ObjCLanguage from Symtab

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.



In D104067#2825199 <https://reviews.llvm.org/D104067#2825199>, @bulbazord wrote:

> ping!

Sorry, feel free to me directly sooner :)

I ran some benchmarks and this changed the average runtime of `InitNameIndexes` from 2.63s to 2.85s when debugging LLDB, which is ok-ish. The overhead seems to be from the language-plugin iteration and the new checks for the different function name types. We can't skip the new function name type checks in the new general approach, but we can avoid the expensive `Language::ForEach` call. If we just once collect the Language plugins in a list and then iterate over that we get down to an average runtime of 2.66s (which is pretty much just what we had before);

  diff --git a/lldb/source/Symbol/Symtab.cpp b/lldb/source/Symbol/Symtab.cpp
  index b61af37a356d..95c57243f20d 100644
  --- a/lldb/source/Symbol/Symtab.cpp
  +++ b/lldb/source/Symbol/Symtab.cpp
  @@ -263,6 +263,13 @@ void Symtab::InitNameIndexes() {
       m_name_indexes_computed = true;
       LLDB_SCOPED_TIMER();
   
  +    // Collect all loaded language plugins.
  +    std::vector<Language *> languages;
  +    Language::ForEach([&languages](Language *l) {
  +      languages.push_back(l);
  +      return true;
  +    });
  +
       auto &name_to_index = GetNameToSymbolIndexMap(lldb::eFunctionNameTypeNone);
       auto &basename_to_index =
           GetNameToSymbolIndexMap(lldb::eFunctionNameTypeBase);
  @@ -329,7 +336,7 @@ void Symtab::InitNameIndexes() {
   
           // If the demangled name turns out to be an ObjC name, and is a category
           // name, add the version without categories to the index too.
  -        auto map_variant_names = [&](Language *lang) {
  +        for (Language *lang : languages) {
             for (auto variant : lang->GetMethodNameVariants(name)) {
               if (variant.GetType() & lldb::eFunctionNameTypeSelector)
                 selector_to_index.Append(variant.GetName(), value);
  @@ -340,10 +347,7 @@ void Symtab::InitNameIndexes() {
               else if (variant.GetType() & lldb::eFunctionNameTypeBase)
                 basename_to_index.Append(variant.GetName(), value);
             }
  -          return true;
           };
  -
  -        Language::ForEach(map_variant_names);
         }
       }
   

Beside that just two nits and this is ready to go. Thanks!



================
Comment at: lldb/include/lldb/Target/Language.h:194
+        : m_name(name), m_type(type) {}
+    ConstString GetName() { return m_name; }
+    lldb::FunctionNameType GetType() { return m_type; }
----------------
Both functions can be const


================
Comment at: lldb/source/Plugins/Language/ObjC/ObjCLanguage.h:103
   //  variant_names[3] => "-[NSString myStringWithCString:]"
-  std::vector<ConstString>
+  // We also return the FunctionNameType of each possible name.
+  std::vector<Language::MethodNameVariant>
----------------
`// Also returns the` (the "We..." isn't something that is usually in doxygen comments which this comment is aspiring to be).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104067/new/

https://reviews.llvm.org/D104067