[PATCH] D126883: [Debuginfo][DWARF][NFC] Refactor DwarfStringPoolEntryRef.

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

In D126883#3583613 <https://reviews.llvm.org/D126883#3583613>, @JDevlieghere wrote:

> Ah that's a good point. Yep the DIEString approach looks good.

I would suggest to use solution from this patch instead of suggestion with DwarfStringPoolEntryRefImpl.

The drawbacks of suggestion with DwarfStringPoolEntryRefImpl:

1. It leads to extra memory usage and fragmentation since many additional objects would be allocated.
2. It requires to change many places where DwarfStringPoolEntryRef is currently used.



  class NonRelocatableStringpool {
    std::unique_ptr<DwarfStringPoolEntryRefImpl> getEntry(StringRef S);
  }
  
  class DIEString {
    std::unique_ptr<DwarfStringPoolEntryRefImpl> S;
  }
  
  class AccelTableBase {
  public:
    /// Represents a group of entries with identical name (and hence, hash value).
    struct HashData {
      std::unique_ptr<DwarfStringPoolEntryRefImpl> Name;
   }
  }
  ...



3. It introduces new performance penalties, because std::unique_ptr de-referencing and virtual function call would be added while accessing DwarfStringPoolEntryRef methods.

The solution from this patch does not introduce new memory allocations, It does not change usage points of DwarfStringPoolEntryRef, It adds only "MapEntry.is<ByValStringEntryPtr>" check while accessing DwarfStringPoolEntryRef methods.

What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126883