Skip to content

Commit 4afb83c

Browse files
committed
Improve type and namespace lookup using .debug_names parent chain
Summary: This diff fixes T199918951 by changing lldb type and namespace lookup to use .debug_names parent chain. It has incorporated latest feedback from Apple/Google. We need to land it internally because: * Unblock Ads partners from dogfooding (currently, they keep on hitting this long delay which disrupts the dogfooding experience) * Dogfooding to flush out any new issues Note: I will keep update the internal patch the same way as the single thread stepping deadlock timeout PR so that we won't get any merge conflict. It includes 4 commits squashed into one diff: * This reverts the internal patch from @WanYi to "Revert PeekDIEName related patch" * Note: `PeekDIEName()` has been removed my the new changes so we won't suffer the original crash anymore. * Improve type lookup review: llvm#108907 * Improve namespace lookup review: llvm#110062 * Use a better algorithm for SameAsEntryContext: Pending =================== Original PR Summary =================== A customer has reported that expanding the "this" pointer for a non-trivial class method context takes more than **70–90 seconds**. This issue occurs on a Linux target with the `.debug_names` index table enabled and using split DWARF (DWO) files to avoid debug info overflow. Profiling shows that the majority of the bottleneck occurs in the `SymbolFileDWARF::FindTypes()` and `SymbolFileDWARF::FindNamespace()` functions, which search for types and namespaces in the `.debug_names` index table by base name, then parse/load the underlying DWO files—an operation that can be very slow. The whole profile trace is pretty big, but here is a part of the hot path: ``` --61.01%-- clang::ASTNodeImporter::VisitRecordDecl(clang::RecordDecl*) --61.00%-- clang::ASTNodeImporter::ImportDeclParts(clang::NamedDecl*, clang::DeclContext*&, clang::DeclContext*&, clang::DeclarationName&, clang::NamedDecl*&, clang::SourceLocation&) clang::ASTImporter::ImportContext(clang::DeclContext*) clang::ASTImporter::Import(clang::Decl*) lldb_private::ClangASTImporter::ASTImporterDelegate::Imported(clang::Decl*, clang::Decl*) lldb_private::ClangASTImporter::BuildNamespaceMap(clang::NamespaceDecl const*) lldb_private::ClangASTSource::CompleteNamespaceMap( std::shared_ptr<std::vector<std::pair<std::shared_ptr<lldb_private::Module>, lldb_private::CompilerDeclContext>, std::allocator<std::pair<std::shared_ptr<lldb_private::Module>, lldb_private::CompilerDeclContext>>> >&, lldb_private::ConstString, std::shared_ptr<std::vector<std::pair<std::shared_ptr<lldb_private::Module>, lldb_private::CompilerDeclContext>, std::allocator<std::pair<std::shared_ptr<lldb_private::Module>, lldb_private::CompilerDeclContext>>> >&) const ) lldb_private::plugin::dwarf::SymbolFileDWARF::FindNamespace(lldb_private::ConstString, lldb_private::CompilerDeclContext const&, bool) lldb_private::plugin::dwarf::DebugNamesDWARFIndex::GetNamespaces(lldb_private::ConstString, llvm::function_ref<bool (lldb_private::plugin::dwarf::DWARFDIE)>) --60.29%-- lldb_private::plugin::dwarf::DebugNamesDWARFIndex::GetDIE(llvm::DWARFDebugNames::Entry const&) const --46.11%-- lldb_private::plugin::dwarf::DWARFUnit::GetDIE(unsigned long) --46.11%-- lldb_private::plugin::dwarf::DWARFUnit::ExtractDIEsIfNeeded() --45.77%-- lldb_private::plugin::dwarf::DWARFUnit::ExtractDIEsRWLocked() --23.22%-- lldb_private::plugin::dwarf::DWARFDebugInfoEntry::Extract(lldb_private::DWARFDataExtractor const&, lldb_private::plugin::dwarf::DWARFUnit const&, unsigned long*) --10.04%-- lldb_private::plugin::dwarf::DWARFFormValue::SkipValue(llvm::dwarf::Form, lldb_private::DWARFDataExtractor const&, unsigned long*, lldb_private::plugin::dwarf::DWARFUnit const*) ``` This PR improves `SymbolFileDWARF::FindTypes()` and `SymbolFileDWARF::FindNamespace()` by utilizing the newly added parent chain `DW_IDX_parent` in `.debug_names`. The proposal was originally discussed in [this RFC](https://discourse.llvm.org/t/rfc-improve-dwarf-5-debug-names-type-lookup-parsing-speed/74151). This PR also introduces a new algorithm in `DebugNamesDWARFIndex::SameAsEntryATName()` that significantly reduces the need to parse DWO files. To leverage the parent chain for `SymbolFileDWARF::FindTypes()` and `SymbolFileDWARF::FindNamespace()`, this PR adds two new APIs: `GetTypesWithParents` and `GetNamespacesWithParents` in the `DWARFIndex` base class. These APIs perform the same function as `GetTypes`/`GetNamespaces`, with additional filtering if the `parent_names` parameter is not empty. Since this only introduces filtering, the callback mechanisms at all call sites remain unchanged. A default implementation in the `DWARFIndex` class parses the type context from each base name matching DIE, then filter by parent_names. In the `DebugNameDWARFIndex` override, the parent_names is cross checked with parent chain in .debug_names for for much faster filtering. Unlike the `GetFullyQualifiedType` API, which fully consumes the `DW_IDX_parent` parent chain for exact matching, these new APIs perform partial subset matching for type/namespace queries. This is necessary to support queries involving anonymous or inline namespaces. For instance, a user might request `NS1::NS2::NS3::Foo`, while the index table's parent chain might contain `NS1::inline_NS2::NS3::Foo`, which would fail exact matching. Another optimization is implemented in `DebugNamesDWARFIndex::SameAsEntryATName()`. The old implementation used `GetNonSkeletonUnit()`, which triggered parsing/loading of underlying DWO files—a very costly operation we aim to avoid. The new implementation minimizes the need to touch DWO files by employing two strategies: 1. Build a `<pool_entry_range => NameEntry>` mapping table (built lazily as needed), allowing us to check the name of the parent entry. 2. Search for the name the entry is trying to match from the current `NameIndex`, then check if the parent entry can be found from the name entry. In my testing and profiling, strategy (1) did not improve wall-time, as the cost of building the mapping table was high. However, strategy (2) proved to be very fast. ========================================================= Test Plan: * `ninja check-lldb` are passing. * It improves T199918951 from around 110s+ => 1.8s. Reviewers: toyang, wanyi, jsamouh, jaihari, jalalonde, #lldb_team Reviewed By: wanyi Subscribers: wanyi, #lldb_team Differential Revision: https://phabricator.intern.facebook.com/D63440813 Tasks: T199918951
1 parent 0630de1 commit 4afb83c

File tree

2 files changed

+66
-14
lines changed

2 files changed

+66
-14
lines changed

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

+62-14
Original file line numberDiff line numberDiff line change
@@ -371,19 +371,56 @@ void DebugNamesDWARFIndex::GetFullyQualifiedType(
371371
m_fallback.GetFullyQualifiedType(context, callback);
372372
}
373373

374+
static CompilerContextKind GetCompilerContextKind(dwarf::Tag tag) {
375+
switch (tag) {
376+
case DW_TAG_namespace:
377+
return CompilerContextKind::Namespace;
378+
case DW_TAG_class_type:
379+
case DW_TAG_structure_type:
380+
return CompilerContextKind::ClassOrStruct;
381+
case DW_TAG_union_type:
382+
return CompilerContextKind::Union;
383+
case DW_TAG_enumeration_type:
384+
return CompilerContextKind::Enum;
385+
case DW_TAG_typedef:
386+
return CompilerContextKind::Typedef;
387+
case DW_TAG_subprogram:
388+
return CompilerContextKind::Function;
389+
case DW_TAG_variable:
390+
return CompilerContextKind::Variable;
391+
case DW_TAG_module:
392+
return CompilerContextKind::Module;
393+
default:
394+
return CompilerContextKind::Any;
395+
}
396+
}
397+
374398
bool DebugNamesDWARFIndex::SameAsEntryContext(
375399
const CompilerContext &query_context,
376400
const DebugNames::Entry &entry) const {
377-
// TODO: check dwarf tag matches.
378-
// Peek at the AT_name of `entry` and test equality to `name`.
401+
CompilerContextKind entry_kind = GetCompilerContextKind(entry.tag());
402+
if ((query_context.kind & entry_kind) == CompilerContextKind::Invalid)
403+
return false;
404+
379405
auto maybe_dieoffset = entry.getDIEUnitOffset();
380406
if (!maybe_dieoffset)
381407
return false;
382-
DWARFUnit *unit = GetNonSkeletonUnit(entry);
383-
if (!unit)
384-
return false;
385-
return query_context.name ==
386-
unit->PeekDIEName(unit->GetOffset() + *maybe_dieoffset);
408+
409+
// [Optimization] instead of parsing the entry from dwo file, we simply
410+
// check if the query_name can point to an entry of the same DIE offset.
411+
// This greatly reduced number of dwo file parsed and thus improved the
412+
// performance.
413+
for (const DebugNames::Entry &query_entry :
414+
entry.getNameIndex()->equal_range(query_context.name)) {
415+
auto query_dieoffset = query_entry.getDIEUnitOffset();
416+
if (!query_dieoffset)
417+
continue;
418+
419+
if (*query_dieoffset == *maybe_dieoffset) {
420+
return true;
421+
}
422+
}
423+
return false;
387424
}
388425

389426
bool DebugNamesDWARFIndex::SameParentChain(
@@ -393,16 +430,27 @@ bool DebugNamesDWARFIndex::SameParentChain(
393430
if (parent_entries.size() != parent_names.size())
394431
return false;
395432

396-
auto SameAsEntryATName = [this](llvm::StringRef name,
397-
const DebugNames::Entry &entry) {
398-
// Peek at the AT_name of `entry` and test equality to `name`.
433+
auto SameAsEntryATName = [](llvm::StringRef name,
434+
const DebugNames::Entry &entry) {
399435
auto maybe_dieoffset = entry.getDIEUnitOffset();
400436
if (!maybe_dieoffset)
401437
return false;
402-
DWARFUnit *unit = GetNonSkeletonUnit(entry);
403-
if (!unit)
404-
return false;
405-
return name == unit->PeekDIEName(unit->GetOffset() + *maybe_dieoffset);
438+
439+
// [Optimization] instead of parsing the entry from dwo file, we simply
440+
// check if the query_name can point to an entry of the same DIE offset.
441+
// This greatly reduced number of dwo file parsed and thus improved the
442+
// performance.
443+
for (const DebugNames::Entry &query_entry :
444+
entry.getNameIndex()->equal_range(name)) {
445+
auto query_dieoffset = query_entry.getDIEUnitOffset();
446+
if (!query_dieoffset)
447+
continue;
448+
449+
if (*query_dieoffset == *maybe_dieoffset) {
450+
return true;
451+
}
452+
}
453+
return false;
406454
};
407455

408456
// If the AT_name of any parent fails to match the expected name, we don't

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

+4
Original file line numberDiff line numberDiff line change
@@ -2789,6 +2789,10 @@ SymbolFileDWARF::FindNamespace(ConstString name,
27892789
if (!DeclContextMatchesThisSymbolFile(parent_decl_ctx))
27902790
return namespace_decl_ctx;
27912791

2792+
std::vector<lldb_private::CompilerContext> parent_compiler_contexts =
2793+
parent_decl_ctx.GetCompilerContext();
2794+
parent_compiler_contexts.push_back({CompilerContextKind::Namespace, name});
2795+
TypeQuery query(parent_compiler_contexts);
27922796
m_index->GetNamespacesWithParents(name, parent_decl_ctx, [&](DWARFDIE die) {
27932797
if (!DIEInDeclContext(parent_decl_ctx, die, only_root_namespaces))
27942798
return true; // The containing decl contexts don't match

0 commit comments

Comments
 (0)