[lldb] add new overload for SymbolFile::FindTypes that accepts a scope

[lassefolger]

Summary

Searching for fully qualified types in a SymbolFile can be inefficient
if it contains many types with the same basename. This is because the
SymbolFile does not offer and an interface to search for qualified
types. To get qualified typed you first need to query all types with the
same basename and then filter based on the scope.

This change introduces a new overload that accepts a scope which the
SymbolFile implementations can use to return filtered by scope.

This change also includes an implementation for DWARF that uses this
scope for filtering. All other implementation use the overload without
scope and filter afterwards.

The motivation for this change is a performance issue when findings
types in targets that contain many types (>1000) with the same basename.
In the two scenarios I tested this change on, the change improves the
time it takes to find a time from 86s -> 12s and from 12s -> 2s,
respectively.

[lassefolger]

rebased on nfc

[werat]

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

2464

Can GetQualifiedName return NULL string?

2464–2465

We can skip this check altogether if the scope is empty.

2465

There's no need to calculate length on every iteration.

Also you can make sc an llvm::StringRef and do something like StringRef(s).startswith(sc).

[shafik]

Is there a specific use case that motivated this feature?

[lassefolger]

In D114627#3162109, @shafik wrote:

Is there a specific use case that motivated this feature?

The motivation is performance. If there are many (>1000) types with the same base name this implementation performs much better than the current implementation.
This is mostly important for the first access (because later ones use the caches results). In our scenarios it reduced the time for the first access from 86s to 6s and from 12s to 2s. And the access for subsequest accesses from 0.15 to 0.08 and 0.12 to 0.06.

The primary reason is that Type::GetQualifiedName is executed for every type with the same basename when the types are filtered at the end. This triggers type completion each type which is very expensive.

I'm still working on this commit.

[lassefolger]

rebased on nfc and completed implementation to expose function in API

[lassefolger]

lldb/source/Symbol/SymbolFile.cpp

148

I need to change this. This was correct in the first iteration but is no longer correct in the current iteration of this change.

[werat]

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

2460

nit: maybe call it scope_ref?

2466

llvm::StringRef(storage) is slightly more efficient here than llvm::StringRef(qn). The latter uses strlen in the constructor to calculate the length of the source string.

[lassefolger]

fixed comment

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

2466

I wasn't sure about the semantics of die.GetQualifiedName().
It does not document whether it's guaranteed that storage contains the results. FWIW, the current implementation does that.
I changed it to use the storage.

[lassefolger]

rebase patch

[werat]

Looks good to me, I'm going to land it soon unless there are objections from other reviewers.

[JDevlieghere]

lldb/include/lldb/Symbol/SymbolFile.h

238–240

242

Does the scope need to be a ConstString? These strings are kept in memory forever and we should be mindful of their use. Could this a StringRef?

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

2465

Can this go into if (has_scope) {?

[shafik]

LGTM after addressing Jonas's comments.

[lassefolger]

update according to comments

lldb/include/lldb/Symbol/SymbolFile.h

242

I don't think it has to be a ConstString. I used a ConstString for consistency with name.
Let me change it to StringRef

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

2465

I don't mind putting it in there but thought reusing the same storage would avoid allocations.
Moving it into the function will lead to an allocation each time the function is called and the qualified name is too long for small string optimization.

[clayborg]

Sorry I am late to making comments on this patch.

This patch wants to add "llvm::StringRef scope" to the FindTypes calls, but this is what "const CompilerDeclContext &parent_decl_ctx" is actually for. If "parent_decl_ctx" is set, it is a declaration context that each type must exist in. So the CompilerDeclContext might be something like "namespace foo" and it might have a parent decl context of "namespace bar". This is currently how filtering types that exist in a decl context works.

The "scope" seems redundant in light of this no? Also, a string based "scope" isn't accurate enough to correctly identify the exact scoping of a type as "a:🅱️:some_type" might be "namespace a -> class b -> some_type" or "namespace a -> namespace b -> some_type".

The one difficult thing about this is currently the "parent_decl_ctx" that is a parameter only works if the decl context comes from the current module. Each module creates types using its own TypeSystem subclass. Those types are vended as CompilerType objects which contain the TypeSystem pointer and a void * for the type. This allows each TypeSystem to create types. For clang compiled code we use lldb_private::TypeSystemClang and we create types as types in a clang::ASTContext. We can also specify declaration contexts by using CompilerDeclContext objects. Each CompilerDeclContext object has a "TypeSystem *" and a "void *" inside of it. Each expression makes its own TypeSystem as well, and one for each expression and one in the target. It should be possible to fix the issue of needing the compiler decl context to come from the current module by having the code that compares the decl context make by using:

ConstString CompilerDeclContext::GetScopeQualifiedName() const;

Or we can modify the CompilerDeclContext class to get the current name:

ConstString CompilerDeclContext::GetName() const;

And then also add an API to return an enumeration for the kind of the declaration context (Namespace, Class, Struct, Union, etc).

[lassefolger]

In D114627#3306300, @clayborg wrote:

Sorry I am late to making comments on this patch.

This patch wants to add "llvm::StringRef scope" to the FindTypes calls, but this is what "const CompilerDeclContext &parent_decl_ctx" is actually for. If "parent_decl_ctx" is set, it is a declaration context that each type must exist in. So the CompilerDeclContext might be something like "namespace foo" and it might have a parent decl context of "namespace bar". This is currently how filtering types that exist in a decl context works.

The "scope" seems redundant in light of this no? Also, a string based "scope" isn't accurate enough to correctly identify the exact scoping of a type as "a:🅱️:some_type" might be "namespace a -> class b -> some_type" or "namespace a -> namespace b -> some_type".

The one difficult thing about this is currently the "parent_decl_ctx" that is a parameter only works if the decl context comes from the current module. Each module creates types using its own TypeSystem subclass. Those types are vended as CompilerType objects which contain the TypeSystem pointer and a void * for the type. This allows each TypeSystem to create types. For clang compiled code we use lldb_private::TypeSystemClang and we create types as types in a clang::ASTContext. We can also specify declaration contexts by using CompilerDeclContext objects. Each CompilerDeclContext object has a "TypeSystem *" and a "void *" inside of it. Each expression makes its own TypeSystem as well, and one for each expression and one in the target. It should be possible to fix the issue of needing the compiler decl context to come from the current module by having the code that compares the decl context make by using:

ConstString CompilerDeclContext::GetScopeQualifiedName() const;

Or we can modify the CompilerDeclContext class to get the current name:

ConstString CompilerDeclContext::GetName() const;

And then also add an API to return an enumeration for the kind of the declaration context (Namespace, Class, Struct, Union, etc).

The idea is that the scope is a hint which implementations can use to improve the performance.
However, I see that this redundancy might be confusing. I have not looked to much into the ComplierDeclContext.
I'll try to take another look later this week.

FWIW there has been a discussion between werat@ and teemperor@ about this on Discord about this topic in general. The results was to use a string rather than a CompilerDeclContext. Although the discussion didn't go into detail why this is:
https://discord.com/channels/636084430946959380/636732809708306432/849218944581632001

[lassefolger]

I'm still planning to look into this just couldn't find the time this week and the last. Sorry for the delay.

[lassefolger]

Sorry for the very late response.

I looked into using the CompilerDeclContext().

This does not work in this case because the context is not set in this specific use case and I'm not sure if it case be set.
It is initialized with an invalid context when the function is called.

The entry point in our use case is lldb::SBTarget::FindTypes(const char *). A stacktrace looks like this:
SymbolFileDWARF::FindTypes()
lldb_private::Module::FindTypes_Impl()
lldb_private::ModuleList::FindTypes()
lldb::SBTarget::FindTypes()

Given that the input is just a C-string, I'm not sure how to derive a CompilerDeclContext() from that.
What we are trying to optimize is a call like:
lldb.target.FindTypes("Some::Namespace::Type")

lldb/include/lldb/Symbol/SymbolFile.h

public:
   virtual void FindFunctions(const RegularExpression &regex,
                              bool include_inlines, SymbolContextList &sc_list);
   virtual void
   FindTypes(ConstString name, const CompilerDeclContext &parent_decl_ctx,
             uint32_t max_matches,
             llvm::DenseSet<lldb_private::SymbolFile *> &searched_symbol_files,
             TypeMap &types);
 
+  // Find types in a specific scope.
+  // \param scope
+  //     Must be either the fully qualified scope (without leading ::) or empty
+  virtual void
+  FindTypes(ConstString name, ConstString scope,
+            const CompilerDeclContext &parent_decl_ctx, uint32_t max_matches,
+            llvm::DenseSet<lldb_private::SymbolFile *> &searched_symbol_files,
+            TypeMap &types);
+
   /// Find types specified by a CompilerContextPattern.
   /// \param languages
   ///     Only return results in these languages.
   /// \param searched_symbol_files
   ///     Prevents one file from being visited multiple times.
   virtual void
   FindTypes(llvm::ArrayRef<CompilerContext> pattern, LanguageSet languages,
             llvm::DenseSet<lldb_private::SymbolFile *> &searched_symbol_files,

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h

void FindFunctions(const lldb_private::RegularExpression &regex,
                      bool include_inlines,
                      lldb_private::SymbolContextList &sc_list) override;
 
   void GetMangledNamesForFunction(
       const std::string &scope_qualified_name,
       std::vector<lldb_private::ConstString> &mangled_names) override;
 
   void
+  FindTypes(lldb_private::ConstString name, lldb_private::ConstString scope,
+            const lldb_private::CompilerDeclContext &parent_decl_ctx,
+            uint32_t max_matches,
+            llvm::DenseSet<lldb_private::SymbolFile *> &searched_symbol_files,
+            lldb_private::TypeMap &types) override;
+
+  void
   FindTypes(lldb_private::ConstString name,
             const lldb_private::CompilerDeclContext &parent_decl_ctx,
             uint32_t max_matches,
             llvm::DenseSet<lldb_private::SymbolFile *> &searched_symbol_files,
             lldb_private::TypeMap &types) override;
 
   void FindTypes(llvm::ArrayRef<lldb_private::CompilerContext> pattern,
                  lldb_private::LanguageSet languages,

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

 #include "lldb/Core/ModuleList.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/Progress.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Core/Value.h"
 #include "lldb/Utility/ArchSpec.h"
+#include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/RegularExpression.h"
 #include "lldb/Utility/Scalar.h"
 #include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/Timer.h"
 
 #include "Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.h"
 #include "Plugins/Language/CPlusPlus/CPlusPlusLanguage.h"
 
void SymbolFileDWARF::GetMangledNamesForFunction(
   }
 }
 
 void SymbolFileDWARF::FindTypes(
     ConstString name, const CompilerDeclContext &parent_decl_ctx,
     uint32_t max_matches,
     llvm::DenseSet<lldb_private::SymbolFile *> &searched_symbol_files,
     TypeMap &types) {
+  FindTypes(name, ConstString(), parent_decl_ctx, max_matches,
+            searched_symbol_files, types);
+}
+
+void SymbolFileDWARF::FindTypes(
+    ConstString name, ConstString scope,
+    const CompilerDeclContext &parent_decl_ctx, uint32_t max_matches,
+    llvm::DenseSet<lldb_private::SymbolFile *> &searched_symbol_files,
+    TypeMap &types) {
   std::lock_guard<std::recursive_mutex> guard(GetModuleMutex());
   // Make sure we haven't already searched this SymbolFile before.
   if (!searched_symbol_files.insert(this).second)
     return;
 
   Log *log(LogChannelDWARF::GetLogIfAll(DWARF_LOG_LOOKUPS));
 
   if (log) {
else
           "SymbolFileDWARF::FindTypes (sc, name=\"%s\", parent_decl_ctx = "
           "NULL, max_matches=%u, type_list)",
           name.GetCString(), max_matches);
   }
 
   if (!DeclContextMatchesThisSymbolFile(parent_decl_ctx))
     return;
 
+  const char *sc = scope.AsCString("");
+  std::string s;
   m_index->GetTypes(name, [&](DWARFDIE die) {
     if (!DIEInDeclContext(parent_decl_ctx, die))
       return true; // The containing decl contexts don't match
 
+    const char *qn = die.GetQualifiedName(s);
+    if (strncmp(sc, qn, strlen(sc)))
+      return true;
+
     Type *matching_type = ResolveType(die, true, true);
     if (!matching_type)
       return true;
 
     // We found a type pointer, now find the shared pointer form our type
     // list
     types.InsertUnique(matching_type->shared_from_this());
     return types.GetSize() < max_matches;

lldb/source/Symbol/SymbolFile.cpp

 }
 
 void SymbolFile::FindTypes(
     ConstString name, const CompilerDeclContext &parent_decl_ctx,
     uint32_t max_matches,
     llvm::DenseSet<lldb_private::SymbolFile *> &searched_symbol_files,
     TypeMap &types) {}
 
+void SymbolFile::FindTypes(
+    ConstString basename, ConstString scope,
+    const CompilerDeclContext &parent_decl_ctx, uint32_t max_matches,
+    llvm::DenseSet<lldb_private::SymbolFile *> &searched_symbol_files,
+    TypeMap &types) {
+  FindTypes(basename, parent_decl_ctx, max_matches, searched_symbol_files,
+            types);
+  TypeClass type_class = eTypeClassAny;
+  types.RemoveMismatchedTypes(std::string(basename.GetCString()),
+                              std::string(scope.GetCString()), type_class,
+                              true);
+}
+
 void SymbolFile::FindTypes(llvm::ArrayRef<CompilerContext> pattern,
                            LanguageSet languages,
                            llvm::DenseSet<SymbolFile *> &searched_symbol_files,
                            TypeMap &types) {}
 
 void SymbolFile::AssertModuleLock() {
   // The code below is too expensive to leave enabled in release builds. It's
   // enabled in debug builds or when the correct macro is set.