[PATCH] D102634: Calculate indexes of last child of each DWARF entry once during tryExtractDIEsIfNeeded.

[Greg Clayton via Phabricator via llvm-commits]

clayborg added a comment.

In D102634#2766582 <https://reviews.llvm.org/D102634#2766582>, @dblaikie wrote:

> What's your use case such that this performance concern has come up for you?

I think anyone who goes to any DIE that contains a bunch of children and uses the DWARFDie::iterator (like for a CU Die) and iterates over its children is probably the worst case.

So any users of DWARFDie::end() like DWARFDie::children(). This is used in many places like in llvm/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp, llvm/lib/CodeGen/AsmPrinter/DIEHash.cpp, llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp, llvm/lib/DWARFLinker/DWARFLinker.cpp and llvm/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp. There may be more and I just searched for "Die.children()".

Any of the DWARFUnit::getSibling(...), DWARFUnit::getParent(...), DWARFUnit::getLastChild(...) would be greatly sped up if we use m_parent_idx  and m_sibling_idx in DWARFDebugInfoEntry.

In D102634#2765564 <https://reviews.llvm.org/D102634#2765564>, @simon.giesecke wrote:

> In D102634#2764987 <https://reviews.llvm.org/D102634#2764987>, @clayborg wrote:
>
>> The DWARFDebugInfoEntry in LLDB contains more data. See inlined comment on improving performance for the LLVM DWARF reader. But if we are going to fix perf issues with DWARFDie navigation, we should improve the DWARFDebugInfoEntry class to contain the information needed to navigate better.
>
> Ok, I added this separately first, because I wasn't sure if it should be optional, and not take up memory for use cases that might not need it. But from your comments I now understand that the extra memory usage should not be an issue.

The LLVM DWARFDebugInfoEntry contains:

  class DWARFDebugInfoEntry {
    /// Offset within the .debug_info of the start of this entry.
    uint64_t Offset = 0;
  
    /// The integer depth of this DIE within the compile unit DIEs where the
    /// compile/type unit DIE has a depth of zero.
    uint32_t Depth = 0;
  
    const DWARFAbbreviationDeclaration *AbbrevDecl = nullptr;
  };

This will end up being 24 bits with 4 bytes of padding after the Depth.

If we modified the LLDB DWARFDebugInfoEntry for LLVM it could contain:

  class DWARFDebugInfoEntry {
    uint64_t Offset; // Offset within the .debug_info/.debug_types
    uint32_t ParentIdx; // How many to subtract from "this" to get the parent. If zero this die has no parent
    uint32_t SiblingIdx; // How many to add to "this" to get the sibling.
    const DWARFAbbreviationDeclaration *AbbrevDecl = nullptr;
  };

This would be the same 24 byte size as the current version and it would improve many of the DWARFUnit::getXXX() calls that get siblings, parents, etc.

So this wouldn't take up any more memory.

>> FYI: be very careful when trying to port any code from the LLDB DWARF parser as it actually removes the NULL terminator DIEs from its DWARFUnit vector of DWARFDebugInfoEntry objects as this saves a ton of memory and they aren't needed if you create your DWARFDebugInfoEntry with enough info.
>
> Should we remove the NULL terminator DIEs here as well, eventually? But I guess even if that should be done, this requires a lot of further adaptations. However, we would probably save more memory than we're adding through the extra sibling and parent index entries.

Again, there will be no memory differences between the old and new due to the padding that exists in the older DWARFDebugInfoEntry structs. I would avoid removing the NULL tags for now because DWARF dumping code relies on these being in there for DWARF printing. This is something we could do later though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102634