[BOLT][NFCI] Use FileSymbols for local symbol disambiguation

[aaupov]

Remove SymbolToFileName mapping from every local symbol to its
containing FILE symbol name, and reuse FileSymbols to disambiguate
local symbols instead.

Also removes the check for ld-temp.o file symbol which was added to
prevent LTO build mode from affecting the disambiguated name. This may
cause incompatibility when using the profile collected on a binary built
in a different mode than the input binary.

Addresses #90661.

[dcci]

What is the tradeoff here? Did we try on large binaries? It looks like you're trading O(1)'ish access for potentially O(N). Can this ever be a problem?

[aaupov]

What is the tradeoff here? Did we try on large binaries? It looks like you're trading O(1)'ish access for potentially O(N). Can this ever be a problem?

The tradeoff is:

• storing a large unordered map Symbol (16b) -> FileName (16b) for each local symbol with set FileName, where we get FileName in O(1) for each local function symbol (i.e. now we store strictly more than needed),
• vs a vector of just FILE symbol data (8b), where we get FileName in O(log FileSyms) * cost of FileSym->getName for each function symbol.

For reference, in one of our medium-sized test binaries we have:

• 6020 local symbols,
• 5841 local function symbols,
• 141 file symbols.

So space savings are quite significant, we gain storage locality, and an extra per-symbol cost is reasonable.

If your concern is about Symbol->getName cost, we can memorize FileName and store pairs of symbol data (8b) and StringRef (16b) instead, reducing the per-symbol cost to just O(log FileSyms).

The switch to storing FILE symbols is a prerequisite for using the symbol table information for split fragment matching.

[dcci]

For reference, in one of our medium-sized test binaries we have:

Can we try on large binaries and try to have an upper bound on the size? I am not against this I just want to make sure we don't regress time dramatically.

The switch to storing FILE symbols is a prerequisite for using the symbol table information for split fragment matching

Thanks for the context. Feel free to add more motivation to your NFC patches/commits, in particular if they're in preparation for something bigger.

[aaupov]

For reference, in one of our medium-sized test binaries we have:

Can we try on large binaries and try to have an upper bound on the size? I am not against this I just want to make sure we don't regress time dramatically.

For one large prod binary, we have the following:

• 2356369 local symbols,
• 1327085 local function symbols,
• 27784 file symbols.

The space tradeoff becomes:

• unordered map: 16b key + 16b value + 8b hash, times # of local symbols => ~90Mb of logical storage
• vector: 8b Symbol data + assuming the memorization of FileName (16b) => ~0.6Mb of logical storage

The switch to storing FILE symbols is a prerequisite for using the symbol table information for split fragment matching

Thanks for the context. Feel free to add more motivation to your NFC patches/commits, in particular if they're in preparation for something bigger.

Done.

[dcci]

For reference, in one of our medium-sized test binaries we have:

Can we try on large binaries and try to have an upper bound on the size? I am not against this I just want to make sure we don't regress time dramatically.

For one large prod binary, we have the following:

• 2356369 local symbols,
• 1327085 local function symbols,
• 27784 file symbols.

What's the BOLT processing time before/after the change?
100MB is a significant amount but how does it compare with BOLT's peak memory usage?

[aaupov]

For reference, in one of our medium-sized test binaries we have:

Can we try on large binaries and try to have an upper bound on the size? I am not against this I just want to make sure we don't regress time dramatically.

For one large prod binary, we have the following:

• 2356369 local symbols,
• 1327085 local function symbols,
• 27784 file symbols.

What's the BOLT processing time before/after the change? 100MB is a significant amount but how does it compare with BOLT's peak memory usage?

It's a drop in the bucket, processing time- and peak memory- wise (~1/1000). Treat this diff as a data structure change to facilitate new use case (which will bring memory consumption back up).

[dcci]

Would be useful, before merging, to see the whole set of patches. The difficulty with tiny NFC patches is that they result in larger picture to be lost, sometimes.

[llvmbot]

@llvm/pr-subscribers-bolt

Author: Amir Ayupov (aaupov)

Changes

In discoverFileObjects, replace mapping from every local symbol to
associated file name with a vector of symbol data for FILE symbols
only. This is a prerequisite for using the symbol table information
for split fragment matching. This also cuts down on memory needed
to resolve local file names.

Test Plan: NFC

---

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

1 Files Affected:

• (modified) bolt/lib/Rewrite/RewriteInstance.cpp (+12-17)

diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp
index 329649c1ca67c5b..8578c2c6c8edcc6 100644
--- a/bolt/lib/Rewrite/RewriteInstance.cpp
+++ b/bolt/lib/Rewrite/RewriteInstance.cpp
@@ -813,14 +813,8 @@ void RewriteInstance::discoverFileObjects() {
 
   // For local symbols we want to keep track of associated FILE symbol name for
   // disambiguation by combined name.
-  StringRef FileSymbolName;
   bool SeenFileName = false;
-  struct SymbolRefHash {
-    size_t operator()(SymbolRef const &S) const {
-      return std::hash<decltype(DataRefImpl::p)>{}(S.getRawDataRefImpl().p);
-    }
-  };
-  std::unordered_map<SymbolRef, StringRef, SymbolRefHash> SymbolToFileName;
+  std::vector<std::pair<DataRefImpl, StringRef>> FileSymbols;
   for (const ELFSymbolRef &Symbol : InputFile->symbols()) {
     Expected<StringRef> NameOrError = Symbol.getName();
     if (NameOrError && NameOrError->starts_with("__asan_init")) {
@@ -847,13 +841,10 @@ void RewriteInstance::discoverFileObjects() {
       // and this uncertainty is causing havoc in function name matching.
       if (Name == "ld-temp.o")
         continue;
-      FileSymbolName = Name;
+      FileSymbols.emplace_back(Symbol.getRawDataRefImpl(), Name);
       SeenFileName = true;
       continue;
     }
-    if (!FileSymbolName.empty() &&
-        !(cantFail(Symbol.getFlags()) & SymbolRef::SF_Global))
-      SymbolToFileName[Symbol] = FileSymbolName;
   }
 
   // Sort symbols in the file by value. Ignore symbols from non-allocatable
@@ -1028,14 +1019,18 @@ void RewriteInstance::discoverFileObjects() {
       // The <id> field is used for disambiguation of local symbols since there
       // could be identical function names coming from identical file names
       // (e.g. from different directories).
-      std::string AltPrefix;
-      auto SFI = SymbolToFileName.find(Symbol);
-      if (SymbolType == SymbolRef::ST_Function && SFI != SymbolToFileName.end())
-        AltPrefix = Name + "/" + std::string(SFI->second);
+      auto CompareSymsByIdx = [](const std::pair<DataRefImpl, StringRef> &A,
+                                 const std::pair<DataRefImpl, StringRef> &B) {
+        return A.first.d.b < B.first.d.b;
+      };
+      DataRefImpl SymDataRef = Symbol.getRawDataRefImpl();
+      auto SFI = llvm::upper_bound(FileSymbols,
+                                   std::make_pair(SymDataRef, StringRef()),
+                                   CompareSymsByIdx);
+      if (SymbolType == SymbolRef::ST_Function && SFI != FileSymbols.begin())
+        AlternativeName = NR.uniquify(Name + "/" + SFI[-1].second.str());
 
       UniqueName = NR.uniquify(Name);
-      if (!AltPrefix.empty())
-        AlternativeName = NR.uniquify(AltPrefix);
     }
 
     uint64_t SymbolSize = ELFSymbolRef(Symbol).getSize();

[aaupov]

For chromium 5735 binary with ~1.2M symbols, discover file objects wall time:

• before: 12.6422s
• after: 12.0297s