[TypeProf][InstrFDO]Omit vtable symbols in indexed profiles by default #96520

mingmingl-llvm · 2024-06-24T17:33:33Z

The indexed iFDO profiles contains compressed vtable names for llvm-profdata show --show-vtables debugging usage. An optimized build doesn't need it (and doesn't decompress the blob) since optimized binary has the source code and IR to find vtable symbols.
The motivation is to avoid increasing profile size when it's not necessary.
This doesn't change the indexed profile format and thereby doesn't need a version change.

- This doesn't change the indexed profile format and thereby doesn't need a version change.

llvmbot · 2024-06-24T17:36:35Z

@llvm/pr-subscribers-pgo

Author: Mingming Liu (minglotus-6)

Changes

The indexed iFDO profiles contains compressed vtable names for llvm-profdata show --show-vtables debugging usage. An optimized build doesn't need it (and doesn't decompress the blob) since optimized binary has the source code and IR to find vtable symbols.
The motivation is to avoid increasing profile size when it's not necessary.
This doesn't change the indexed profile format and thereby doesn't need a version change.

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

2 Files Affected:

(modified) llvm/test/tools/llvm-profdata/vtable-value-prof.test (+2-2)
(modified) llvm/tools/llvm-profdata/llvm-profdata.cpp (+9-4)

diff --git a/llvm/test/tools/llvm-profdata/vtable-value-prof.test b/llvm/test/tools/llvm-profdata/vtable-value-prof.test
index 378c2e11b236b..8dc8f6f0d480e 100644
--- a/llvm/test/tools/llvm-profdata/vtable-value-prof.test
+++ b/llvm/test/tools/llvm-profdata/vtable-value-prof.test
@@ -1,7 +1,7 @@
 ; RUN: rm -rf %t && mkdir %t && cd %t
 
 ; Generate indexed profiles from text profiles
-RUN: llvm-profdata merge %S/Inputs/vtable-value-prof.proftext -o indexed.profdata
+RUN: llvm-profdata merge --keep-vtable-symbols %S/Inputs/vtable-value-prof.proftext -o indexed.profdata
 
 ; Show indexed profiles
 RUN: llvm-profdata show --function=main --ic-targets --show-vtables indexed.profdata | FileCheck %s --check-prefix=INDEXED
@@ -10,7 +10,7 @@ RUN: llvm-profdata show --function=main --ic-targets --show-vtables indexed.prof
 RUN: llvm-profdata show --function=main --ic-targets --show-vtables --text %S/Inputs/vtable-value-prof.proftext | FileCheck %s --check-prefix=ICTEXT
 
 ; Convert indexed profiles to its textual output and show it.
-RUN: llvm-profdata merge --text -o text-from-indexed.proftext indexed.profdata
+RUN: llvm-profdata merge --keep-vtable-symbols --text -o text-from-indexed.proftext indexed.profdata
 RUN: llvm-profdata show --function=main --ic-targets --show-vtables text-from-indexed.proftext | FileCheck %s --check-prefix=INDEXED
 RUN: llvm-profdata show --function=main --ic-targets --show-vtables --text text-from-indexed.proftext | FileCheck %s --check-prefix=ICTEXT
 
diff --git a/llvm/tools/llvm-profdata/llvm-profdata.cpp b/llvm/tools/llvm-profdata/llvm-profdata.cpp
index fae6d1e989ab5..8a9345920f71e 100644
--- a/llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ b/llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -291,6 +291,10 @@ cl::opt<bool> DropProfileSymbolList(
     cl::desc("Drop the profile symbol list when merging AutoFDO profiles "
              "(only meaningful for -sample)"));
 
+cl::opt<bool> KeepVTableSymbols(
+    "keep-vtable-symbols", cl::init(false), cl::Hidden,
+    cl::desc("If true, keep the vtable symbols in indexed profiles"));
+
 // Temporary support for writing the previous version of the format, to enable
 // some forward compatibility.
 // TODO: Consider enabling this with future version changes as well, to ease
@@ -767,11 +771,12 @@ static void loadInput(const WeightedFile &Input, SymbolRemapper *Remapper,
     });
   }
 
-  const InstrProfSymtab &symtab = Reader->getSymtab();
-  const auto &VTableNames = symtab.getVTableNames();
+  if (KeepVTableSymbols) {
+    const InstrProfSymtab &symtab = Reader->getSymtab();
+    const auto &VTableNames = symtab.getVTableNames();
 
-  for (const auto &kv : VTableNames) {
-    WC->Writer.addVTableName(kv.getKey());
+    for (const auto &kv : VTableNames)
+      WC->Writer.addVTableName(kv.getKey());
   }
 
   if (Reader->hasTemporalProfile()) {

teresajohnson · 2024-06-25T14:37:36Z

The indexed iFDO profiles contains compressed vtable names

To clarify, are we currently including both compressed and uncompressed vtable names in the indexed profiles?

david-xl

How much overhead does it add to the indexed profile?

mingmingl-llvm · 2024-06-25T18:36:33Z

The indexed iFDO profiles contains compressed vtable names

To clarify, are we currently including both compressed and uncompressed vtable names in the indexed profiles?

Not really. Either zlib-compressed or non-compressed vtable names are stored, depending on compression::zlib::isAvailable return value around these lines (

llvm-project/llvm/lib/ProfileData/InstrProfWriter.cpp

Lines 809 to 814 in fef144c

    
           std::string CompressedVTableNames; 
        
           if (!VTableNameStrs.empty()) 
        
             if (Error E = collectGlobalObjectNameStrings( 
        
                     VTableNameStrs, compression::zlib::isAvailable(), 
        
                     CompressedVTableNames)) 
        
               return E;

).

InstrProfWriter::writeVTableNames (source) will write the vtable payload in the following format

length of the payload (source)
one std::string named CompressedVTableNames (source).
padding (source; this mostly follow the binary-id section)

With this change --keep-vtable-symbols=true, the CompressedVTableNames is empty, length is zero and there is no padding. But the indexed profile format remains unchanged.

mingmingl-llvm · 2024-06-25T18:45:05Z

How much overhead does it add to the indexed profile?

I have a raw profile at hand for a search binary with large ELF size. Its raw profile size is 507M.

With --keep-vtable-symbol=true, indexed profile profile.profdata is 901M.
With --keep-vtable-symbol=false, indexed profie profile.profdata is 893M.

So this is ~8M out of ~900M; but something nice to have (e.g., for deployment, and for faster indexed profile generation).

p.s. I'm doing a wider range of testing for --enable-vtable-value-profiling so could gather profile size diff for other binaries as a side work.

mingmingl-llvm · 2024-06-25T20:41:11Z

To clarify, are we currently including both compressed and uncompressed vtable names in the indexed profiles?

I forgot to mention that LLVM_ENABLE_ZLIB is by default enabled if zlib is found (https://llvm.org/docs/CMake.html). Our internal toolchains turns this option on and generated compressed names.

I have seen DLLVM_ENABLE_ZLIB=0 on some buildbots. IR tests need to requires: zlib to be unsupported on those buildbots.

david-xl

Perhaps make the option default to true initially?

teresajohnson · 2024-06-25T22:38:40Z

To clarify, are we currently including both compressed and uncompressed vtable names in the indexed profiles?

Nevermind, I misunderstood. We have either compressed or uncompressed names - and with this change we won't emit either (iiuc).

How much overhead does it add to the indexed profile?

I have a raw profile at hand for a search binary with large ELF size. Its raw profile size is 507M.

With --keep-vtable-symbol=true, indexed profile profile.profdata is 901M.

With --keep-vtable-symbol=false, indexed profie profile.profdata is 893M.

So this is ~8M out of ~900M; but something nice to have (e.g., for deployment, and for faster indexed profile generation).

That's a fairly small decrease, but otoh I don't see a big downside since the names are there in the raw profile which is often available.

teresajohnson

lgtm but see if @david-xl still has concerns

david-xl · 2024-06-25T22:41:26Z

no real concerns.

mingmingl-llvm · 2024-06-26T03:27:51Z

Perhaps make the option default to true initially?

Hmm, I'm hoping build tools or profile processing tools can pick up --keep-vtable-symbols=false naturally to save the need to update them individually. Let me know if there is a scenario or example we'd prefer to override the option in separate changes!

With --keep-vtable-symbols=false, there won't be issues when merging old profiles (generated before this change) and new profiles (generated with this change).

and with this change we won't emit either (iiuc).

Correct!

That's a fairly small decrease, but otoh I don't see a big downside since the names are there in the raw profile which is often available.

I gathered profiles from 4 other binaries. The decrease is smaller than 1% across binaries.

binary	Diff	profile size before (in MB)	profile size after (in MB)
search1	0.8%	337	334
search2	0.9%	330	327
database	0.5%	256.7	255.3
network	0.6%	98.4	97.8

mingmingl-llvm · 2024-06-26T18:30:14Z

Let me know if there is a scenario or example we'd prefer to override the option in separate changes!

I'm going to land this change with --keep-vtable-symbols=false after the following tests

With prior released llvm-profdata as cli1, llvm-profdata built with this PR as cli2.

Usecli2 to generated profiles and use cli1 to consume (merge, show, overlap) them.
Use cli1 to generate profiles and use cli2 to consume them.

Both ways things work.

llvm#96520) - The indexed iFDO profiles contains compressed vtable names for `llvm-profdata show --show-vtables` debugging usage. An optimized build doesn't need it and doesn't decompress the blob now [1], since optimized binary has the source code and IR to find vtable symbols. - The motivation is to avoid increasing profile size when it's not necessary. - This doesn't change the indexed profile format and thereby doesn't need a version change. [1] https://github.com/llvm/llvm-project/blob/eac925fb81f26342811ad1765e8f9919628e2254/llvm/include/llvm/ProfileData/InstrProfReader.h#L696-L699

[TypeProf][InstrFDO]Omit vtable symbols in indexed profiles by default

bb035c6

- This doesn't change the indexed profile format and thereby doesn't need a version change.

mingmingl-llvm requested review from david-xl, snehasish, teresajohnson and modiking June 24, 2024 17:33

mingmingl-llvm marked this pull request as ready for review June 24, 2024 17:36

llvmbot added the PGO Profile Guided Optimizations label Jun 24, 2024

fix compiler-rt test

70b90f8

llvmbot added the compiler-rt label Jun 24, 2024

make a merge subcommand

9e37451

david-xl reviewed Jun 25, 2024

View reviewed changes

mingmingl-llvm requested a review from david-xl June 25, 2024 18:45

david-xl reviewed Jun 25, 2024

View reviewed changes

teresajohnson approved these changes Jun 25, 2024

View reviewed changes

david-xl approved these changes Jun 25, 2024

View reviewed changes

mingmingl-llvm merged commit 3f78d89 into llvm:main Jun 26, 2024
7 checks passed

mingmingl-llvm deleted the omitpayload branch June 26, 2024 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[TypeProf][InstrFDO]Omit vtable symbols in indexed profiles by default #96520

[TypeProf][InstrFDO]Omit vtable symbols in indexed profiles by default #96520

Uh oh!

mingmingl-llvm commented Jun 24, 2024 •

edited

Loading

Uh oh!

llvmbot commented Jun 24, 2024

Uh oh!

teresajohnson commented Jun 25, 2024

Uh oh!

david-xl left a comment

Uh oh!

mingmingl-llvm commented Jun 25, 2024

Uh oh!

mingmingl-llvm commented Jun 25, 2024

Uh oh!

mingmingl-llvm commented Jun 25, 2024

Uh oh!

david-xl left a comment

Uh oh!

teresajohnson commented Jun 25, 2024

Uh oh!

teresajohnson left a comment

Uh oh!

david-xl commented Jun 25, 2024

Uh oh!

mingmingl-llvm commented Jun 26, 2024

Uh oh!

mingmingl-llvm commented Jun 26, 2024

Uh oh!

Uh oh!

Uh oh!

[TypeProf][InstrFDO]Omit vtable symbols in indexed profiles by default #96520

[TypeProf][InstrFDO]Omit vtable symbols in indexed profiles by default #96520

Uh oh!

Conversation

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

Uh oh!

Uh oh!

llvmbot commented Jun 24, 2024

Uh oh!

teresajohnson commented Jun 25, 2024

Uh oh!

david-xl left a comment

Choose a reason for hiding this comment

Uh oh!

mingmingl-llvm commented Jun 25, 2024

Uh oh!

mingmingl-llvm commented Jun 25, 2024

Uh oh!

mingmingl-llvm commented Jun 25, 2024

Uh oh!

david-xl left a comment

Choose a reason for hiding this comment

Uh oh!

teresajohnson commented Jun 25, 2024

Uh oh!

teresajohnson left a comment

Choose a reason for hiding this comment

Uh oh!

david-xl commented Jun 25, 2024

Uh oh!

mingmingl-llvm commented Jun 26, 2024

Uh oh!

mingmingl-llvm commented Jun 26, 2024

Uh oh!

Uh oh!

Uh oh!

mingmingl-llvm commented Jun 24, 2024 •

edited

Loading