[LTO][ELF][lld] Use unique string saver in ELF bitcode symbol parsing #106670

mingmingl-llvm · 2024-08-30T06:13:23Z

lld ELF BitcodeFile uses string saver to keep copies of bitcode symbols. Symbol duplication is very common when compiling application binaries.

This change proposes to introduce a UniqueStringSaver in lld context and use it for bitcode symbol parsing. The implementation covers ELF only. Similar opportunities should exist on other (COFF, MachO, wasm) formats.

For an internal production binary where lto indexing takes ~10GiB originally, this changes optimizes away ~800MiB (~7.8%), measured by https://github.com/google/pprof. Flame graph breaks down memory by usage call stacks and agrees with this measurement.

llvmbot · 2024-08-30T06:48:49Z

@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-elf

Author: Mingming Liu (minglotus-6)

Changes

lld ELF BitcodeFile uses string saver to keep copies of bitcode symbols. Symbol duplication is very common when compiling application binaries.

This change proposes to introduce a UniqueStringSaver in lld context and use it for bitcode symbol parsing. The implementation covers ELF only. Similar opportunities should exist on other (COFF, MachO, wasm) formats.

For an internal production binary where lto indexing takes ~10GiB originally, this changes optimizes away ~800MiB (~7.8%), measured by https://github.com/google/pprof. Flame graph breaks down memory usage call stacks and agrees with this measurement.

Full diff: https://github.com/llvm/llvm-project/pull/106670.diff

3 Files Affected:

(modified) lld/ELF/InputFiles.cpp (+20-4)
(modified) lld/ELF/InputFiles.h (+10)
(modified) lld/include/lld/Common/CommonLinkerContext.h (+5)

diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index 7adc35f20984a5..ed946a91404088 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -1695,6 +1695,22 @@ static uint8_t getOsAbi(const Triple &t) {
   }
 }
 
+StringRef BitcodeFile::saved_symbol(const char *S) {
+  return unique_saver().save(S);
+}
+
+StringRef BitcodeFile::saved_symbol(StringRef S) {
+  return unique_saver().save(S);
+}
+
+StringRef BitcodeFile::saved_symbol(const Twine &S) {
+  return unique_saver().save(S);
+}
+
+StringRef BitcodeFile::saved_symbol(const std::string &S) {
+  return unique_saver().save(S);
+}
+
 BitcodeFile::BitcodeFile(MemoryBufferRef mb, StringRef archiveName,
                          uint64_t offsetInArchive, bool lazy)
     : InputFile(BitcodeKind, mb) {
@@ -1712,8 +1728,8 @@ BitcodeFile::BitcodeFile(MemoryBufferRef mb, StringRef archiveName,
   // symbols later in the link stage). So we append file offset to make
   // filename unique.
   StringRef name = archiveName.empty()
-                       ? saver().save(path)
-                       : saver().save(archiveName + "(" + path::filename(path) +
+                       ? saved_symbol(path)
+                       : saved_symbol(archiveName + "(" + path::filename(path) +
                                       " at " + utostr(offsetInArchive) + ")");
   MemoryBufferRef mbref(mb.getBuffer(), name);
 
@@ -1745,7 +1761,7 @@ createBitcodeSymbol(Symbol *&sym, const std::vector<bool> &keptComdats,
   uint8_t visibility = mapVisibility(objSym.getVisibility());
 
   if (!sym)
-    sym = symtab.insert(saver().save(objSym.getName()));
+    sym = symtab.insert(unique_saver().save(objSym.getName()));
 
   int c = objSym.getComdatIndex();
   if (objSym.isUndefined() || (c != -1 && !keptComdats[c])) {
@@ -1797,7 +1813,7 @@ void BitcodeFile::parseLazy() {
   symbols = std::make_unique<Symbol *[]>(numSymbols);
   for (auto [i, irSym] : llvm::enumerate(obj->symbols()))
     if (!irSym.isUndefined()) {
-      auto *sym = symtab.insert(saver().save(irSym.getName()));
+      auto *sym = symtab.insert(saved_symbol(irSym.getName()));
       sym->resolve(LazySymbol{*this});
       symbols[i] = sym;
     }
diff --git a/lld/ELF/InputFiles.h b/lld/ELF/InputFiles.h
index b0a74877d0aaeb..28c4c1a1f92962 100644
--- a/lld/ELF/InputFiles.h
+++ b/lld/ELF/InputFiles.h
@@ -9,6 +9,8 @@
 #ifndef LLD_ELF_INPUT_FILES_H
 #define LLD_ELF_INPUT_FILES_H
 
+#include <string>
+
 #include "Config.h"
 #include "Symbols.h"
 #include "lld/Common/ErrorHandler.h"
@@ -23,6 +25,8 @@
 namespace llvm {
 struct DILineInfo;
 class TarWriter;
+class StringRef;
+class Twine;
 namespace lto {
 class InputFile;
 }
@@ -335,6 +339,12 @@ class BitcodeFile : public InputFile {
   void postParse();
   std::unique_ptr<llvm::lto::InputFile> obj;
   std::vector<bool> keptComdats;
+
+private:
+  StringRef saved_symbol(const char *S);
+  StringRef saved_symbol(StringRef S);
+  StringRef saved_symbol(const Twine &S);
+  StringRef saved_symbol(const std::string &S);
 };
 
 // .so file.
diff --git a/lld/include/lld/Common/CommonLinkerContext.h b/lld/include/lld/Common/CommonLinkerContext.h
index 0627bbdc8bd877..0c5349fbf16cd1 100644
--- a/lld/include/lld/Common/CommonLinkerContext.h
+++ b/lld/include/lld/Common/CommonLinkerContext.h
@@ -38,6 +38,7 @@ class CommonLinkerContext {
 
   llvm::BumpPtrAllocator bAlloc;
   llvm::StringSaver saver{bAlloc};
+  llvm::UniqueStringSaver unique_saver{bAlloc};
   llvm::DenseMap<void *, SpecificAllocBase *> instances;
 
   ErrorHandler e;
@@ -56,6 +57,10 @@ bool hasContext();
 
 inline llvm::StringSaver &saver() { return context().saver; }
 inline llvm::BumpPtrAllocator &bAlloc() { return context().bAlloc; }
+
+inline llvm::UniqueStringSaver &unique_saver() {
+  return context().unique_saver;
+}
 } // namespace lld
 
 #endif

teresajohnson · 2024-08-30T15:05:35Z

lld/ELF/InputFiles.cpp

-                       ? saver().save(path)
-                       : saver().save(archiveName + "(" + path::filename(path) +
+                       ? saved_symbol(path)
+                       : saved_symbol(archiveName + "(" + path::filename(path) +


Why not just use unique_saver().save() directly like you do below on line 1764?

I was thinking of BitcodeFile::saved_symbol as helper functions to make it more explicit that bitcode file uses unique saver.

Now I use saver() or unique_saver() from linker context directly (for simpler code) and add comment around to explain the motivation.

teresajohnson · 2024-08-30T15:07:02Z

lld/include/lld/Common/CommonLinkerContext.h

@@ -56,6 +57,10 @@ bool hasContext();

 inline llvm::StringSaver &saver() { return context().saver; }


Does it make sense to just switch this one to UniqueStringSaver? That would automatically provide the same benefit to other clients like COFF and MachO

UniqueStringSaver allocates a DenseMap<StringRef> for de-duplication, and the dense-set has a memory overhead when the saved strings don't get duplicated. Symbols get duplicated (due to #include and linkonce_odr) a lot to offset the dense-set overhead, while other saved strings (e.g., file paths) don't get much duplication. Presumably the number of duplicated symbols are large enough so that switching this one to unique saver is more memory efficient than HEAD but less memory efficient than using unique saver selectively for symbols (experiment data below)

Besides, the current saver are used in a couple of places (in lld, llvm-objdump, etc) and it'd be easier to use a couple of incremental changes to make every user of saver compile (to reap the memory saving for indexing first) even if there are more places where unique saver is better. I added a FIXME in CommonLinkerContext.h to ook into other places where duplications are common in saved strings and unique saver make sense.

Experiment Data

Experiment data are collected from two benchmarks x three set-ups, showing that selectively uniqufy symbols has a small win over switching.

The peak indexing memory, measured before indexing returns

llvm-project/lld/ELF/LTO.cpp

Lines 356 to 357 in 77f0488

indexFile->close();

return {};

benchmark search1 search2

HEAD 3.4GiB 9.89GiB

switching to unique saver 3.37GiB 8.79GiB

use unique saver for BitcodeFile methods 3.30GiB 8.48GiB

The peak memory under lld::elf::parseFiles (where bitcode parsing happens), collected from the same run.

benchmark search1 search2

HEAD 1.1GiB 2.79GiB

switching to unique saver 921MiB 2.01GiB

use unique saver for BitcodeFile methods 911MiB 1.991GiB

kazutakahirata · 2024-08-30T19:09:54Z

lld/ELF/InputFiles.h

@@ -335,6 +339,12 @@ class BitcodeFile : public InputFile {
  void postParse();
  std::unique_ptr<llvm::lto::InputFile> obj;
  std::vector<bool> keptComdats;
+
+private:
+  StringRef saved_symbol(const char *S);


I realize Teresa has already suggested you to use unique_saver().save() directly. Even if you keep these saved_symbol, the one that takes cons char * should be redundant because we can StringRef has an implicit constructor from const char *.

The same applies to the one below that takes const std::string &.

I actually take back what I said above. When you have std::string, it can be implicitly cast to either StringRef or const Twine &, causing ambiguity. The same story probably applies to const char *.

Use savers in linker context and undo header changes.

kazutakahirata · 2024-08-30T19:11:04Z

lld/ELF/InputFiles.h

@@ -9,6 +9,8 @@
 #ifndef LLD_ELF_INPUT_FILES_H
 #define LLD_ELF_INPUT_FILES_H

+#include <string>


Once you remove StringRef saved_symbol(const std::string &S); below, you should be able to remove this include also.

[lld][ELF]Use unique saver in bitcode symbol parsing

7074440

mingmingl-llvm marked this pull request as ready for review August 30, 2024 06:48

mingmingl-llvm requested review from MaskRay and teresajohnson August 30, 2024 06:48

llvmbot added lld lld:ELF labels Aug 30, 2024

mingmingl-llvm requested a review from kazutakahirata August 30, 2024 06:48

mingmingl-llvm changed the title ~~[LTO][ELF][lld] Use unique saver in ELF bitcode symbol parsing~~ [LTO][ELF][lld] Use unique string saver in ELF bitcode symbol parsing Aug 30, 2024

teresajohnson reviewed Aug 30, 2024

View reviewed changes

kazutakahirata reviewed Aug 30, 2024

View reviewed changes

resolve review comments

86f030e

mingmingl-llvm requested review from teresajohnson and kazutakahirata September 5, 2024 07:47

mingmingl-llvm mentioned this pull request Sep 5, 2024

[NFCI][LTO][lld] Optimize away symbol copies within LTO global resolution in ELF #106193

Merged

teresajohnson approved these changes Sep 5, 2024

View reviewed changes

mingmingl-llvm merged commit 64498c5 into llvm:main Sep 5, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[LTO][ELF][lld] Use unique string saver in ELF bitcode symbol parsing #106670

[LTO][ELF][lld] Use unique string saver in ELF bitcode symbol parsing #106670

Uh oh!

mingmingl-llvm commented Aug 30, 2024 •

edited

Loading

Uh oh!

llvmbot commented Aug 30, 2024 •

edited

Loading

Uh oh!

teresajohnson Aug 30, 2024

Uh oh!

mingmingl-llvm Sep 5, 2024

Uh oh!

teresajohnson Aug 30, 2024

Uh oh!

mingmingl-llvm Sep 5, 2024

Uh oh!

kazutakahirata Aug 30, 2024

Uh oh!

kazutakahirata Aug 30, 2024

Uh oh!

mingmingl-llvm Sep 5, 2024

Uh oh!

kazutakahirata Aug 30, 2024

Uh oh!

mingmingl-llvm Sep 5, 2024

Uh oh!

Uh oh!

Uh oh!

		@@ -56,6 +57,10 @@ bool hasContext();

		inline llvm::StringSaver &saver() { return context().saver; }

benchmark	search1	search2
HEAD	3.4GiB	9.89GiB
switching to unique saver	3.37GiB	8.79GiB
use unique saver for BitcodeFile methods	3.30GiB	8.48GiB

benchmark	search1	search2
HEAD	1.1GiB	2.79GiB
switching to unique saver	921MiB	2.01GiB
use unique saver for BitcodeFile methods	911MiB	1.991GiB

[LTO][ELF][lld] Use unique string saver in ELF bitcode symbol parsing #106670

[LTO][ELF][lld] Use unique string saver in ELF bitcode symbol parsing #106670

Uh oh!

Conversation

mingmingl-llvm commented Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Experiment Data

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mingmingl-llvm commented Aug 30, 2024 •

edited

Loading

llvmbot commented Aug 30, 2024 •

edited

Loading