[PATCH] D126059: [Debuginfo][DWARF][NFC] Add paired methods working with DWARFDebugInfoEntry.

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

In D126059#3528520 <https://reviews.llvm.org/D126059#3528520>, @clayborg wrote:

> I would actually request that no one can play with DWARFDebugInfoEntry items at all except for the DWARFDie class. The main issue is you can ask the wrong DWARFUnit class to do something with a DWARFDebugInfoEntry object that it doesn't own. We ran into this problem in LLDB and this is why we created the DWARFDie class in LLDB. If you encapsulate everything into the DWARFDie class and control all entry points, then this mistake can not be made since everyone needs to use the DWARFDie class. If you do want it add it, I would suggest adding an assert that verifies that the DWARFDebugInfoEntry belongs to the current DWARFUnit in each new function that takes a "DWARFDebugInfoEntry *", but I would still rather see everyone use DWARFDie if possible for safety reason and to avoid having an assert in each call.

The reason to use DWARFDebugInfoEntry is the performance concern.
The non optimized DWARF for clang binary has more than 160 million dies:

  llvm-dwarfdump --debug-info clang | grep -c DW_TAG 
  162452994

There are a many methods which create dies, f.e.:

  DWARFDie getDIEAtIndex(unsigned Index) {
    return DWARFDie(this, getDebugInfoEntry(Index));
  }
  
  DWARFDie getDIEForOffset(uint64_t Offset) {
    if (Optional<uint32_t> DieIdx = getDIEIndexForOffset(Offset))
      return DWARFDie(this, &DieArray[*DieIdx]);
  
    return DWARFDie();
  }

...

these methods are called for each die more than once, this causes the DWARFDie class constructor to be called a very large number of times.
While using DWARFDebugInfoEntry is cheap. They are allocated once at DieArray and later are passed as the pointers(No need to call constructor).

Other than that, passing DWARFDie by value leads to copying more data - 16 bytes instead of 8 bytes for DWARFDebugInfoEntry*.

All above lead to noticable performance impact.

If we can agree on using asserts to check that the proper unit is used(as you suggested) then I propose to use that approach.
Will update this review to use assertions checking DWARFUnit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126059