[lld][COFF] Remove duplicate strtab entries #141197

HaohaiWen · 2025-05-23T04:42:15Z

String table size is too big for large binary when symbol table is
enabled. Some strings in strtab is same so it can be reused.

This patch revives 9ffeaaa authored by mstorsjo with the prioritized
string table builder to fix debug section name issue (see 4d2eda2
for more details).

String table size is too big for large binary when symbol table is enabled. Some strings in strtab is same so it can be reused.

llvmbot · 2025-05-23T04:42:47Z

@llvm/pr-subscribers-llvm-binary-utilities
@llvm/pr-subscribers-mc
@llvm/pr-subscribers-platform-windows

@llvm/pr-subscribers-lld-coff

Author: Haohai Wen (HaohaiWen)

Changes

String table size is too big for large binary when symbol table is
enabled. Some strings in strtab is same so it can be reused.

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

1 Files Affected:

(modified) lld/COFF/Writer.cpp (+8-4)

diff --git a/lld/COFF/Writer.cpp b/lld/COFF/Writer.cpp
index db6133e20a037..6ec83a5f77e5a 100644
--- a/lld/COFF/Writer.cpp
+++ b/lld/COFF/Writer.cpp
@@ -285,6 +285,7 @@ class Writer {
   std::unique_ptr<FileOutputBuffer> &buffer;
   std::map<PartialSectionKey, PartialSection *> partialSections;
   std::vector<char> strtab;
+  StringMap<size_t> strtabMap;
   std::vector<llvm::object::coff_symbol16> outputSymtab;
   std::vector<ECCodeMapEntry> codeMap;
   IdataContents idata;
@@ -1439,10 +1440,13 @@ void Writer::assignOutputSectionIndices() {
 
 size_t Writer::addEntryToStringTable(StringRef str) {
   assert(str.size() > COFF::NameSize);
-  size_t offsetOfEntry = strtab.size() + 4; // +4 for the size field
-  strtab.insert(strtab.end(), str.begin(), str.end());
-  strtab.push_back('\0');
-  return offsetOfEntry;
+  size_t newOffsetOfEntry = strtab.size() + 4; // +4 for the size field
+  auto res = strtabMap.try_emplace(str, newOffsetOfEntry);
+  if (res.second) {
+    strtab.insert(strtab.end(), str.begin(), str.end());
+    strtab.push_back('\0');
+  }
+  return res.first->getValue();
 }
 
 std::optional<coff_symbol16> Writer::createSymbol(Defined *def) {

llvmbot · 2025-05-23T04:42:48Z

@llvm/pr-subscribers-lld

Author: Haohai Wen (HaohaiWen)

Changes

String table size is too big for large binary when symbol table is
enabled. Some strings in strtab is same so it can be reused.

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

1 Files Affected:

(modified) lld/COFF/Writer.cpp (+8-4)

diff --git a/lld/COFF/Writer.cpp b/lld/COFF/Writer.cpp
index db6133e20a037..6ec83a5f77e5a 100644
--- a/lld/COFF/Writer.cpp
+++ b/lld/COFF/Writer.cpp
@@ -285,6 +285,7 @@ class Writer {
   std::unique_ptr<FileOutputBuffer> &buffer;
   std::map<PartialSectionKey, PartialSection *> partialSections;
   std::vector<char> strtab;
+  StringMap<size_t> strtabMap;
   std::vector<llvm::object::coff_symbol16> outputSymtab;
   std::vector<ECCodeMapEntry> codeMap;
   IdataContents idata;
@@ -1439,10 +1440,13 @@ void Writer::assignOutputSectionIndices() {
 
 size_t Writer::addEntryToStringTable(StringRef str) {
   assert(str.size() > COFF::NameSize);
-  size_t offsetOfEntry = strtab.size() + 4; // +4 for the size field
-  strtab.insert(strtab.end(), str.begin(), str.end());
-  strtab.push_back('\0');
-  return offsetOfEntry;
+  size_t newOffsetOfEntry = strtab.size() + 4; // +4 for the size field
+  auto res = strtabMap.try_emplace(str, newOffsetOfEntry);
+  if (res.second) {
+    strtab.insert(strtab.end(), str.begin(), str.end());
+    strtab.push_back('\0');
+  }
+  return res.first->getValue();
 }
 
 std::optional<coff_symbol16> Writer::createSymbol(Defined *def) {

MaskRay · 2025-05-23T05:26:16Z

Test needed. In ELF we can use llvm-readelf -p .strtab to dump the section table. I don't know what the COFF counterpart is.

mstorsjo · 2025-05-23T06:58:12Z

Instead of just using StringMap to deduplicate whole strings, it's also possible to use the StringTableBuilder class for deduplicating and doing tail merging of strings - optimizing the string table even further (at the cost of a little longer link time).

This was attempted before in 9ffeaaa, but I had to revert it in 4d2eda2; see the revert commit for an explanation of the issues I ran into, including a suggestion on what one could do to reapply it.

Anyway, using a StringMap probably is lower overhead than using StringTableBuilder, and it keeps the strings in their current order (i.e. the early section names don't end up too far back in the string table).

Do you have any numbers on how much binary size you save with this? It could be interesting to compare with the StringTableBuilder case.

HaohaiWen · 2025-05-23T07:20:12Z

Instead of just using std::map to deduplicate whole strings, it's also possible to use the StringTableBuilder class for deduplicating and doing tail merging of strings - optimizing the string table even further (at the cost of a little longer link time).

This was attempted before in 9ffeaaa, but I had to revert it in 4d2eda2; see the revert commit for an explanation of the issues I ran into, including a suggestion on what one could do to reapply it.

Anyway, using a std::map probably is lower overhead than using StringTableBuilder, and it keeps the strings in their current order (i.e. the early section names don't end up too far back in the string table).

Do you have any numbers on how much binary size you save with this? It could be interesting to compare with the StringTableBuilder case.

Thanks for reminding.
The blender binary size has been reduced about 300MB. There are many duplicate symbol name.

HaohaiWen · 2025-05-23T07:43:47Z

Instead of just using std::map to deduplicate whole strings, it's also possible to use the StringTableBuilder class for deduplicating and doing tail merging of strings - optimizing the string table even further (at the cost of a little longer link time).

This was attempted before in 9ffeaaa, but I had to revert it in 4d2eda2; see the revert commit for an explanation of the issues I ran into, including a suggestion on what one could do to reapply it.

Anyway, using a std::map probably is lower overhead than using StringTableBuilder, and it keeps the strings in their current order (i.e. the early section names don't end up too far back in the string table).

Do you have any numbers on how much binary size you save with this? It could be interesting to compare with the StringTableBuilder case.

How about using two StringTableBuilder, one for long section name and another for others?
I have observed large strtab size many times for large projects.
Will you revive your patch with supporting for prioritized long section name?

mstorsjo · 2025-05-23T08:59:57Z

How about using two StringTableBuilder, one for long section name and another for others? I have observed large strtab size many times for large projects. Will you revive your patch with supporting for prioritized long section name?

I don't plan on reviving it myself right now, but if you're working in this area, I would appreciate if you'd try to reapply that patch (revert the revert) and see if it can be made to work again on current git main. Then with that in place, it would be interesting to see if you see any further significant size reductions on your blender testcase.

If it doesn't really reduce the binary size any much further (perhaps cases where tail merging makes any difference are rare?), then perhaps it's best to just go with the much simpler approach of your current patch here.

HaohaiWen · 2025-05-26T02:06:34Z

Blender strtab size for three methods.

original strtab size	stringmap strtab size	strtabbuilder strtab size
630,474,756	373,099,443	342,999,672

stringmap reduced -40.82% comparing to unoptimized strtab.
stringtablebuilder further reduced -8.07%.

mstorsjo · 2025-05-26T12:16:25Z

Blender strtab size for three methods.

original strtab size stringmap strtab size strtabbuilder strtab size
630,474,756 373,099,443 342,999,672
stringmap reduced -40.82% comparing to unoptimized strtab. stringtablebuilder further reduced -8.07%.

Thanks for looking into these numbers!

So using the StringTableBuilder does make a notable extra benefit on top of this, but using it would require a bit more extra effort than what I tried before.

With that in mind, I think this PR in itself looks like a straightforward improvement. We could consider switching to StringTableBuilder, if someone is willing to make the effort to handle the section name strings separately in one way or another - but it is a smaller improvement than this, and probably comes at a bigger runtime cost.

Test needed. In ELF we can use llvm-readelf -p .strtab to dump the section table. I don't know what the COFF counterpart is.

Not sure if I think a test strictly is needed here; whether the strings are deduplicated or not is mostly a nonfunctional difference. (The previous similar change in 9ffeaaa didn't have any test.) I'm not aware of any current llvm-readobj command or similar, that would actually dump the string table as such. (Also obj2yaml doesn't dump the string table, but only uses implicitly for resolving strings for all the referenced symbols.)

HaohaiWen · 2025-05-27T06:40:23Z

Let's first support --string-table for COFF.
#141552

HaohaiWen · 2025-05-28T02:58:17Z

Let's first support --string-table for COFF. #141552

Test will be added after landing of this PR.

HaohaiWen · 2025-05-28T02:59:52Z

Blender strtab size for three methods.

original strtab size stringmap strtab size strtabbuilder strtab size
630,474,756 373,099,443 342,999,672
stringmap reduced -40.82% comparing to unoptimized strtab. stringtablebuilder further reduced -8.07%.

strtab size after this PR: 342,999,676

HaohaiWen · 2025-06-09T08:55:16Z

Any comments?

mstorsjo · 2025-06-09T09:00:48Z

I think this looks reasonable, thanks! I'd prefer to have an ack from @MaskRay for the additions to the shared MC level StringTableBuilder class though, but if he doesn't follow up in a few days I guess I can approve this as well.

MaskRay

The MC looks good, modulo some comments. LGTM once @mstorsjo approves.

MaskRay · 2025-06-13T03:13:11Z

llvm/lib/MC/StringTableBuilder.cpp

    std::vector<StringPair *> Strings;
    Strings.reserve(StringIndexMap.size());
    for (StringPair &P : StringIndexMap)
      Strings.push_back(&P);

-    multikeySort(Strings, 0);
+    auto getStringPriority = [this](const CachedHashStringRef Str) -> uint8_t {
+      if (StringPriorityMap.contains(Str))


StringPriorityMap.lookup(Str)

This lambda can be moved inside if (StringPriorityMap.size()) {

MaskRay · 2025-06-13T03:18:58Z

llvm/include/llvm/MC/StringTableBuilder.h

+  /// table is finalized. Priority is only useful with reordering. Strings with
+  /// same priority will be put together. Strings with higher priority are
+  /// placed closer to the begin of string table.
+  LLVM_ABI size_t add(CachedHashStringRef S, uint8_t Priority = 0);


This comment should mention what if a string is added with different priorities. (std::max below)

MaskRay · 2025-06-13T03:22:48Z

llvm/lib/MC/StringTableBuilder.cpp

+                 });
+      // Make sure ArrayRef is valid. Although std::sort implementaion is
+      // normally in-place , it is not guaranteed by standard.
+      StringsRef = Strings;


Seems unnecessary. std::sort will sort the elements within the range. If StringRef applies to the whole range, it will not be invalidated even if std::sort requires external storage (internally).

mstorsjo

LGTM for the lld side.

HaohaiWen · 2025-06-17T05:27:50Z

ping @MaskRay

llvm/include/llvm/MC/StringTableBuilder.h

jh7370 · 2025-06-17T06:44:28Z

llvm/lib/ObjCopy/COFF/COFFWriter.cpp

@@ -121,7 +121,7 @@ void COFFWriter::layoutSections() {
 Expected<size_t> COFFWriter::finalizeStringTable() {
  for (const auto &S : Obj.getSections())
    if (S.Name.size() > COFF::NameSize)
-      StrTabBuilder.add(S.Name);
+      StrTabBuilder.add(S.Name, /*Priority=*/UINT8_MAX);


This feels like an unrelated change. Is it even necessary? If it is important, should it have a test and should it be in its own PR?

This is the same potential issue like 4d2eda2.
We need to put long section name in the front of string table.

That's fine, but a) the PR title implies this PR is for lld, so it needs updating or this bit splitting into a second PR and b) llvm-objcopy needs a test that shows that this bit of code has been added.

Address comments Co-authored-by: James Henderson <[email protected]>

HaohaiWen · 2025-06-20T03:18:01Z

@MaskRay, any more comments?
Let's land it if no objection.

chapuni · 2025-06-21T13:55:59Z

lld/test/COFF/strtab.s

@@ -0,0 +1,29 @@
+# RUN: llvm-mc -triple=x86_64-windows-msvc %s -filetype=obj -o %t.obj


Does it work if x86 is not configured?

It does not: https://lab.llvm.org/buildbot/#/builders/190/builds/21872

I have marked the test as requiring x86 in 757c80d.

Thanks! I didn't aware of that.

String table size is too big for large binary when symbol table is enabled. Some strings in strtab is same so it can be reused. This patch revives 9ffeaaa authored by mstorsjo with the prioritized string table builder to fix debug section name issue (see 4d2eda2 for more details). --------- Co-authored-by: Wen Haohai <[email protected]> Co-authored-by: James Henderson <[email protected]>

[lld][COFF] Remove duplicate strtab entries

4204ca0

String table size is too big for large binary when symbol table is enabled. Some strings in strtab is same so it can be reused.

HaohaiWen requested a review from williamweixiao May 23, 2025 04:42

llvmbot added lld lld:COFF platform:windows labels May 23, 2025

HaohaiWen requested a review from mstorsjo May 23, 2025 04:44

williamweixiao requested a review from MaskRay May 23, 2025 12:56

Using prioritized StringTableBuilder

a4a6c91

llvmbot added the mc Machine (object) code label May 28, 2025

HaohaiWen added 3 commits June 3, 2025 09:03

Merge remote-tracking branch 'origin/main' into strtab

8e034a8

Add test

47e5b2c

Fix ObjCopy

333a908

llvmbot added the llvm:binary-utilities label Jun 5, 2025

Merge remote-tracking branch 'origin/main' into strtab

1f428ce

MaskRay reviewed Jun 13, 2025

View reviewed changes

Address comments

bd00aba

HaohaiWen requested a review from MaskRay June 16, 2025 07:47

mstorsjo approved these changes Jun 16, 2025

View reviewed changes

jh7370 reviewed Jun 17, 2025

View reviewed changes

HaohaiWen and others added 2 commits June 17, 2025 14:58

Update llvm/include/llvm/MC/StringTableBuilder.h

ff8fce2

Address comments Co-authored-by: James Henderson <[email protected]>

split llvm-copy changes to another PR

ac31e68

williamweixiao approved these changes Jun 18, 2025

View reviewed changes

MaskRay approved these changes Jun 20, 2025

View reviewed changes

HaohaiWen merged commit 9cc9efc into llvm:main Jun 21, 2025
7 checks passed

HaohaiWen deleted the strtab branch June 21, 2025 05:44

chapuni reviewed Jun 21, 2025

View reviewed changes

		@@ -0,0 +1,29 @@
		# RUN: llvm-mc -triple=x86_64-windows-msvc %s -filetype=obj -o %t.obj

[lld][COFF] Remove duplicate strtab entries #141197

[lld][COFF] Remove duplicate strtab entries #141197

Uh oh!

Conversation

HaohaiWen commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented May 23, 2025

Uh oh!

MaskRay commented May 23, 2025

Uh oh!

mstorsjo commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HaohaiWen commented May 23, 2025

Uh oh!

HaohaiWen commented May 23, 2025

Uh oh!

mstorsjo commented May 23, 2025

Uh oh!

HaohaiWen commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mstorsjo commented May 26, 2025

Uh oh!

HaohaiWen commented May 27, 2025

Uh oh!

HaohaiWen commented May 28, 2025

Uh oh!

HaohaiWen commented May 28, 2025

Uh oh!

HaohaiWen commented Jun 9, 2025

Uh oh!

mstorsjo commented Jun 9, 2025

Uh oh!

MaskRay left a comment

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!

mstorsjo left a comment

Choose a reason for hiding this comment

Uh oh!

HaohaiWen commented Jun 17, 2025

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!

HaohaiWen commented Jun 20, 2025

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

Uh oh!

Uh oh!

HaohaiWen commented May 23, 2025 •

edited

Loading

llvmbot commented May 23, 2025 •

edited

Loading

mstorsjo commented May 23, 2025 •

edited

Loading

HaohaiWen commented May 26, 2025 •

edited

Loading