[PATCH] D139603: [llvm-profdata] Add option to cap profile output size

[William Junda Huang via Phabricator via llvm-commits]

huangjd added inline comments.


================
Comment at: llvm/lib/ProfileData/SampleProfWriter.cpp:567
+    const SampleProfileMap &ProfileMap) {
+  NameTable.clear();
+  for (const auto &I : ProfileMap) {
----------------
davidxl wrote:
> The clear call changes the behavior. Why is it needed?
Name table should only contains names exist in the current ProfileMap. The original implementation adds to the name table when writing a new profile, and the old names are never cleared, which is actually a bug. (However SampleProfileWriter is single use, an instance never calls write twice, so this bug was not showing up until this new feature) 


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:1089-1091
+      size_t NumToRemove = CalculateNumFunctionsToRemove(
+          OriginalFunctionCount, Functions->size(),
+          Writer->getOutputStream().tell(), OutputSizeLimit);
----------------
wenlei wrote:
> This heuristic can be quite inaccurate. The number of body sample + call site sample entries can be much better proxy for actual profile size and yet still easily accessible. 
> 
> `functionSamples.getBodySamples().size() + functionSamples.getCallsiteSamples().size()`
The new revision performs the iterations on a string buffer so the performance of the heuristic is not a big concern (and now it should not overshoot by reducing too many functions). Although  using a simple proportional heuristic is still too slow because it ends up reducing one function at the tail at a time


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139603/new/

https://reviews.llvm.org/D139603