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

[huangjd]

Summary

D139603 (add option to llvm-profdata to reduce output profile size) contains test cases that are not cross-platform. Moving those tests to unit test and making sure the feature is callable from llvm library

[snehasish]

lgtm with some comments.

llvm/lib/ProfileData/SampleProfWriter.cpp

100

This will become unused in non-DEBUG build, maybe add the workaround here https://reviews.llvm.org/rG9f4a9d3f44501fa755eb71fe855e15cf0e59e8b8

Or wrap this in LLVM_DEBUG too.

Also IterationCount below as mentioned in this comment on the previous revision: https://reviews.llvm.org/D139603#inline-1365026

llvm/unittests/tools/llvm-profdata/OutputSizeLimitTest.cpp

41

I think we should replace report_fatal_error with

std::error_code EC = ReaderOrErr.getError();
ASSERT_FALSE(EC) << EC.message().c_str();

to let gtest handle the failure cleanly.

69

Perhaps TestWriteWithSizeLimit instead of TestOutputSizeLimit1 is a little more informative?

71

This should probably be EXPECT_LE.
The rationale is explained here https://stackoverflow.com/a/2565309

[huangjd]

Updating D141446: Fix to D139603(reverted) - moved size check to unit test so that it is cross-platform

[huangjd]

Updating D141446: Fix to D139603(reverted) - moved size check to unit test so that it is cross-platform

[vvereschaka]

Hi @huangjd ,

check-llvm gets failed with the following linker errors:

FAILED: unittests/tools/llvm-profdata/LLVMProfdataTests 
: && /usr/bin/c++ -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG -fuse-ld=gold     -Wl,--gc-sections unittests/tools/llvm-profdata/CMakeFiles/LLVMProfdataTests.dir/OutputSizeLimitTest.cpp.o -o unittests/tools/llvm-profdata/LLVMProfdataTests  -Wl,-rpath,/home/buildbot/worker/as-builder-7/llvm-nvptx-nvidia-ubuntu/build/lib  lib/libLLVMProfileData.so.16git  lib/libllvm_gtest_main.so.16git  lib/libLLVMTestingSupport.so.16git  lib/libllvm_gtest.so.16git  lib/libLLVMSupport.so.16git  -Wl,-rpath-link,/home/buildbot/worker/as-builder-7/llvm-nvptx-nvidia-ubuntu/build/lib && :
unittests/tools/llvm-profdata/CMakeFiles/LLVMProfdataTests.dir/OutputSizeLimitTest.cpp.o:OutputSizeLimitTest.cpp:function TestOutputSizeLimit_TestOutputSizeLimit1_Test::TestBody() [clone .localalias]: error: undefined reference to 'llvm::LLVMContext::LLVMContext()'
unittests/tools/llvm-profdata/CMakeFiles/LLVMProfdataTests.dir/OutputSizeLimitTest.cpp.o:OutputSizeLimitTest.cpp:function TestOutputSizeLimit_TestOutputSizeLimit1_Test::TestBody() [clone .localalias]: error: undefined reference to 'llvm::LLVMContext::~LLVMContext()'
collect2: error: ld returned 1 exit status

https://lab.llvm.org/buildbot/#/builders/234/builds/2655

Would you take a look?

[thakis]

This unit-test based test also fails on non-linux, see e.g. http://45.33.8.238/macm1/52570/step_11.txt

I think that's because writeWithSizeLimitInternal calls Strategy->Erase(StringBuffer.size()); and that erases an element from an unordered_map, and it depends on the C++ standard library if an "important" function name gets removed. Depending on that, the error is either too_large (which the test expects) or truncated_name_table (which it doesn't).

Please take a look and revert for now if it takes a while to fix. If you do end up reverting, it'd be cool if you could revert 6be251352e6b4d9708a1b7b7b146ea199342de22 in the same commit (and reland it when you reland).

llvm/unittests/tools/llvm-profdata/OutputSizeLimitTest.cpp

37

https://github.com/google/googletest/blob/main/docs/advanced.md#propagating-fatal-failures FYI

[dyung]

We are also seeing an assertion failure in the added unit test on our internal Windows builder similar to what this bot is hitting:

https://lab.llvm.org/buildbot/#/builders/231/builds/7174

[ RUN      ] TestOutputSizeLimit.TestOutputSizeLimit1
LLVMProfdataTests: /home/buildbots/ppc64be-clang-test-suite/clang-ppc64be-test-suite/llvm-project/llvm/lib/ProfileData/SampleProfWriter.cpp:85: virtual void llvm::sampleprof::DefaultFunctionPruningStrategy::Erase(size_t): Assertion `NumToRemove <= SortedFunctions.size()' failed.

[vitalybuka]

This bot is still broken https://lab.llvm.org/buildbot/#/builders/168/builds/11266

[huangjd]

Updated unit test

[huangjd]

Updated unit test

[vvereschaka]

I have tested the latest changes on llvm-nvptx-nvidia-ubuntu (https://lab.llvm.org/buildbot/#/builders/234) builder locally and the unit tests get built and run successfully.
Thank you @huangjd.

[huangjd]

Update unit test, previously may crash on mac OS

[huangjd]

Refactored to fix bugs on non linux platforms

Moved all the tests into unit test since

1. Output size limit doesn't require the preservation of specific functions if a different pruning strategy is used in the future, so CHECK-NEXT is too restrictive. In unit test each rewritten sample is checked against the original sample to confirm all its fields are identical

2. Windows use '\r\n' line ending so the behavior of output size limit in text mode is different (more functions being pruned than linux for the same limit because extra char per line). This breaks lit test

3. Write once for many tests with repetative output

[huangjd]

cleanup

[huangjd]

use llvm::unittest::TempFile instead

[huangjd]

@snehasish Please re-review since I introduced a major change to the code.

[snehasish]

lgtm

llvm/lib/ProfileData/SampleProfWriter.cpp

111

Is this leaking the raw_svector_ostream objects?

Can we rewrite this as the following?

raw_svector_ostream OS(StringBuffer);
OutputStream.reset(&OS);

Also might be worth it to use a separate OutputStream object within the loop scope so that we don't have to worry about the swap before and after and the lifetimes of the raw_vector_ostream objects.

llvm/unittests/tools/llvm-profdata/OutputSizeLimitTest.cpp

71

I think these parameters name should be capitalized based on the guide.
https://llvm.org/docs/CodingStandards.html

Also consider moving this to the implementation of FunctionSamples since it seems generally useful to have operator== implemented?

173

VAR_RETURN_IF_ERROR can be used here?

197

EXPECT_THAT_EXPECTED is probably better to continue with other test cases rather than aborting the test on the first failure?

[huangjd]

llvm/lib/ProfileData/SampleProfWriter.cpp

111

unique_ptr.reset(new obj) is the correct usage.
https://en.cppreference.com/w/cpp/memory/unique_ptr/reset

[snehasish]

llvm/lib/ProfileData/SampleProfWriter.cpp

111

Thanks, knowing the type makes it clear! Perhaps that is additional motivation for a separate, local OutputStream var?

[huangjd]

llvm/unittests/tools/llvm-profdata/OutputSizeLimitTest.cpp

197

The callee returns an error only if there's something wrong with I/O or profile reading, which is not expected to happen at all, so ASSERT is used properly here. This test checks the correctness of the sample map after size reduction, which is checked with EXPECT_EQ

[huangjd]

Cleanup unit test

[huangjd]

update tests because API change from main

llvm/lib/ProfileData/SampleProfWriter.cpp

 }
 
 std::error_code SampleProfileWriter::writeWithSizeLimitInternal(
     SampleProfileMap &ProfileMap, size_t OutputSizeLimit,
     FunctionPruningStrategy *Strategy) {
   if (OutputSizeLimit == 0)
     return write(ProfileMap);
 
   size_t OriginalFunctionCount = ProfileMap.size();
 
   SmallVector<char> StringBuffer;
   std::unique_ptr<raw_ostream> BufferStream(
       new raw_svector_ostream(StringBuffer));
   OutputStream.swap(BufferStream);
 
   if (std::error_code EC = write(ProfileMap))
     return EC;
   size_t IterationCount = 0;
   while (StringBuffer.size() > OutputSizeLimit) {
     Strategy->Erase(StringBuffer.size());
 
     if (ProfileMap.size() == 0)
       return sampleprof_error::too_large;
 
     StringBuffer.clear();
     OutputStream.reset(new raw_svector_ostream(StringBuffer));
     if (std::error_code EC = write(ProfileMap))
       return EC;
     IterationCount++;
   }
 
   OutputStream.swap(BufferStream);
   OutputStream->write(StringBuffer.data(), StringBuffer.size());
   LLVM_DEBUG(dbgs() << "Profile originally has " << OriginalFunctionCount
                     << " functions, reduced to " << ProfileMap.size() << " in "
                     << IterationCount << " iterations\n");
+  // Silence warning on Release build.
+  (void)OriginalFunctionCount;
+  (void)IterationCount;
   return sampleprof_error::success;
 }
 
 std::error_code
 SampleProfileWriter::writeFuncProfiles(const SampleProfileMap &ProfileMap) {
   std::vector<NameFunctionSamples> V;
   sortFuncProfiles(ProfileMap, V);
   for (const auto &I : V) {

llvm/unittests/tools/llvm-profdata/OutputSizeLimitTest.cpp

10: inline1:1000
   1: 1000
  10: inline2:2000
   1: 2000
 _Z3bari:20301:1437
  1: 1437
 _Z3fooi:7711:610
  1: 610)";
 
+static void CheckAssertFalse(std::error_code t) {
+  ASSERT_FALSE(t) << t.message().c_str();
+}
+
 static SampleProfileMap ReadInput(StringRef Input) {
   LLVMContext Context;
   auto InputBuffer = MemoryBuffer::getMemBufferCopy(Input);
   auto ReaderOrErr = SampleProfileReader::create(InputBuffer, Context);
-  if (std::error_code EC = ReaderOrErr.getError())
-    report_fatal_error(EC.message().c_str());
+  CheckAssertFalse(ReaderOrErr.getError());
   auto Reader = std::move(ReaderOrErr.get());
-  if (std::error_code EC = Reader->read())
-    report_fatal_error(EC.message().c_str());
+  CheckAssertFalse(Reader->read());
   return Reader->getProfiles();
 }
 
 static std::unique_ptr<SampleProfileWriter>
 CreateWriter(SmallVector<char> &OutputBuffer) {
   std::unique_ptr<raw_ostream> BufferStream(
       new raw_svector_ostream(OutputBuffer));
   auto WriterOrErr = SampleProfileWriter::create(
       BufferStream, llvm::sampleprof::SPF_Ext_Binary);
-  if (std::error_code EC = WriterOrErr.getError())
-    report_fatal_error(EC.message().c_str());
+  CheckAssertFalse(WriterOrErr.getError());
   return std::move(WriterOrErr.get());
 }
 
-// Only returns the actual size of the written profile.
+// Returns the actual size of the written profile.
 static size_t WriteProfile(StringRef Input, size_t SizeLimit) {
   SampleProfileMap Profiles = ReadInput(Input);
   SmallVector<char> OutputBuffer;
   auto Writer = CreateWriter(OutputBuffer);
-  if (std::error_code EC = Writer->writeWithSizeLimit(Profiles, SizeLimit))
-    report_fatal_error(EC.message().c_str());
+  std::error_code EC = Writer->writeWithSizeLimit(Profiles, SizeLimit);
+  // too_large means no sample could be written because SizeLimit is too small.
+  // Otherwise any other error code indicates unexpected failure.
+  if (EC == sampleprof_error::too_large)
+    return 0;
+  else
+    CheckAssertFalse(EC);
   return OutputBuffer.size();
 }
 
 TEST(TestOutputSizeLimit, TestOutputSizeLimit1) {
-  for (size_t OutputSizeLimit : {489, 488, 474})
-    ASSERT_LE(WriteProfile(Input1, OutputSizeLimit), OutputSizeLimit);
+  for (size_t OutputSizeLimit : {489, 488, 474, 400})
+    EXPECT_LE(WriteProfile(Input1, OutputSizeLimit), OutputSizeLimit);
 }