ELF: Use bump pointer allocator for uncompressed section buffers. NFCI.

[pcc]

Summary

This shaves another word off SectionBase and makes it possible to clone a
section using the implicit copy constructor.

This basically reverts r311056, which removed the mutex in order to
make the code easier to understand. On balance I think it's probably more
straightforward to have a mutex here than to have an unusual copy constructor
in SectionBase.

[ruiu]

I wonder how much does it actually cost to acquire a lock under the situation where there's no contention. Except this place, all accesses to BAlloc are already serialized, so there's usually no contention. If the cost of acquiring a lock under that situation is negligible, I'd imagine that always acquiring the lock could be a reasonable option.

[pcc]

In D59269#1426604, @ruiu wrote:

I wonder how much does it actually cost to acquire a lock under the situation where there's no contention. Except this place, all accesses to BAlloc are already serialized, so there's usually no contention. If the cost of acquiring a lock under that situation is negligible, I'd imagine that always acquiring the lock could be a reasonable option.

The only other user of BAlloc is StringSaver (all other allocations use a SpecificBumpPtrAllocator). Are you proposing to change BumpPtrAllocator::Allocate to take a lock?

[ruiu]

I thought that we could protect BumpPtrAllocator::Allocate with a mutex, but since StringSaver directly calls that function, there's no way to acquire a lock before calling that function. Maybe we should protect StringSaver::save? Or, we could also make BAlloc and StringSaver thread-local variables. That might be a better approach.

[pcc]

In D59269#1426677, @ruiu wrote:

I thought that we could protect BumpPtrAllocator::Allocate with a mutex, but since StringSaver directly calls that function, there's no way to acquire a lock before calling that function. Maybe we should protect StringSaver::save?

We could, I suppose? But then that code would be effectively dead since we don't save any strings on threads other than the main thread right now. I'm more inclined to let TSan find any mistakes involving BumpPtrAllocator here.

Or, we could also make BAlloc and StringSaver thread-local variables. That might be a better approach.

I'm not sure if that will work. Imagine that we spin up a thread that decompresses a section and then terminates. When the thread terminates, the allocator is destroyed, so we are left with a dangling pointer in the section object.

We could work around that by having the thread local variables be pointers to objects allocated on the heap, and arrange for them to be destroyed later in lld::freeArena. But now we're back to the original problem of thread-safely adding a "thing to deallocate later" to a list.

[ruiu]

In D59269#1426696, @pcc wrote:

In D59269#1426677, @ruiu wrote:

I thought that we could protect BumpPtrAllocator::Allocate with a mutex, but since StringSaver directly calls that function, there's no way to acquire a lock before calling that function. Maybe we should protect StringSaver::save?

We could, I suppose? But then that code would be effectively dead since we don't save any strings on threads other than the main thread right now. I'm more inclined to let TSan find any mistakes involving BumpPtrAllocator here.

Or, we could also make BAlloc and StringSaver thread-local variables. That might be a better approach.

I'm not sure if that will work. Imagine that we spin up a thread that decompresses a section and then terminates. When the thread terminates, the allocator is destroyed, so we are left with a dangling pointer in the section object.

We could work around that by having the thread local variables be pointers to objects allocated on the heap, and arrange for them to be destroyed later in lld::freeArena. But now we're back to the original problem of thread-safely adding a "thing to deallocate later" to a list.

Hmm, you are right. My approach won't work. The other thing I can think of is to make BumpPtrAllocator thread-safe and mostly lock-free using compare-and-exchange. That doesn't seem technically not too hard, but that's probably too much. I think I'm fine with your original approach.

LGTM

[MaskRay]

This patch decreases memory usage by 1% when linking a huge internal target with tcmalloc-based lld. Nice job! (However, on the other hand, it makes early freeing pattern impossible; doing that might have been rejected, though)

[grimar]

ELF/InputSection.h

217 ↗

(On Diff #190320)

Not relative to this patch, but I wonder why this is an int64_t and not a uint64_t.

[ruiu]

ELF/InputSection.h

217 ↗

(On Diff #190320)

To represent -1 as -1, I guess?

[grimar]

ELF/InputSection.h

217 ↗

(On Diff #190320)

But it could be uint64_t, we already have code around in linker that does that.
E.g:

// Get an offset in an output section this relocation is applied to.
uint64_t Offset = GetOffset.get(Rel.r_offset);
if (Offset == uint64_t(-1))
  return;

I am not sure it is useful atm to have a max size of UncompressedSize to be max(uint64)-1
instead of max(int64_t), but why not...

[ruiu]

ELF/InputSection.h

217 ↗

(On Diff #190320)

I'm not too worried, but if you want to consistently use uint64_t, feel free to do that. By the way uint64_t(-1) has the same bit pattern as UINT64_MAX.

lld/ELF/InputSection.h

public:
   // its static type.
   InputFile *File;
 
   template <class ELFT> ObjFile<ELFT> *getFile() const {
     return cast_or_null<ObjFile<ELFT>>(File);
   }
 
   ArrayRef<uint8_t> data() const {
-    if (UncompressedSize >= 0 && !UncompressedBuf)
+    if (UncompressedSize >= 0)
       uncompress();
     return RawData;
   }
 
   uint64_t getOffsetInFile() const;
 
   // Input sections are part of an output section. Special sections
   // like .eh_frame and merge sections are first combined into a
public:
   }
 
 protected:
   void parseCompressedHeader();
   void uncompress() const;
 
   mutable ArrayRef<uint8_t> RawData;
 
-  // A pointer that owns uncompressed data if a section is compressed by zlib.
-  // Since the feature is not used often, this is usually a nullptr.
-  mutable std::unique_ptr<char[]> UncompressedBuf;
-  int64_t UncompressedSize = -1;
+  // This field stores the uncompressed size of the compressed data in RawData,
+  // or -1 if RawData is not compressed (either because the section wasn't
+  // compressed in the first place, or because we ended up uncompressing it).
+  // Since the feature is not used often, this is usually -1.
+  mutable int64_t UncompressedSize = -1;
 };
 
 // SectionPiece represents a piece of splittable section contents.
 // We allocate a lot of these and binary search on them. This means that they
 // have to be as compact as possible, which is why we don't store the size (can
 // be found by looking at the next one).
 struct SectionPiece {
   SectionPiece(size_t Off, uint32_t Hash, bool Live)

lld/ELF/InputSection.cpp

if (auto *S = dyn_cast<SyntheticSection>(this))
     return S->getSize();
   if (UncompressedSize >= 0)
     return UncompressedSize;
   return RawData.size();
 }
 
 void InputSectionBase::uncompress() const {
   size_t Size = UncompressedSize;
-  UncompressedBuf.reset(new char[Size]);
+  char *UncompressedBuf;
+  {
+    static std::mutex Mu;
+    std::lock_guard<std::mutex> Lock(Mu);
+    UncompressedBuf = BAlloc.Allocate<char>(Size);
+  }
 
-  if (Error E =
-          zlib::uncompress(toStringRef(RawData), UncompressedBuf.get(), Size))
+  if (Error E = zlib::uncompress(toStringRef(RawData), UncompressedBuf, Size))
     fatal(toString(this) +
           ": uncompress failed: " + llvm::toString(std::move(E)));
-  RawData = makeArrayRef((uint8_t *)UncompressedBuf.get(), Size);
+  RawData = makeArrayRef((uint8_t *)UncompressedBuf, Size);
+  UncompressedSize = -1;
 }
 
 uint64_t InputSectionBase::getOffsetInFile() const {
   const uint8_t *FileStart = (const uint8_t *)File->MB.getBufferStart();
   const uint8_t *SecStart = data().begin();
   return SecStart - FileStart;
 }
 
template <class ELFT> void InputSection::writeTo(uint8_t *Buf) {
   // If -r is given, we may have a SHT_GROUP section.
   if (Type == SHT_GROUP) {
     copyShtGroup<ELFT>(Buf + OutSecOff);
     return;
   }
 
   // If this is a compressed section, uncompress section contents directly
   // to the buffer.
-  if (UncompressedSize >= 0 && !UncompressedBuf) {
+  if (UncompressedSize >= 0) {
     size_t Size = UncompressedSize;
     if (Error E = zlib::uncompress(toStringRef(RawData),
                                    (char *)(Buf + OutSecOff), Size))
       fatal(toString(this) +
             ": uncompress failed: " + llvm::toString(std::move(E)));
     uint8_t *BufEnd = Buf + OutSecOff + Size;
     relocate<ELFT>(Buf, BufEnd);
     return;