[llvm-profdata] Use simple enum for error checking in Sample Profile Reader

[huangjd]

Benchmarking data show that around 7% of the time is spent on handling llvm::ErrorOr and std::error_code, which are not efficiently processed. This has a significant impact on profile load time, so it should be changed into a simple enum and check if it is 0 instead.

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff f70963c69d0278e2b9d958196e72fb8cc5e9afde 228eabe8a2c102b6ddaaa804b4264e54c98cb93d -- llvm/include/llvm/ProfileData/SampleProf.h llvm/include/llvm/ProfileData/SampleProfReader.h llvm/lib/ProfileData/SampleProfReader.cpp

View the diff from clang-format here.

diff --git a/llvm/include/llvm/ProfileData/SampleProfReader.h b/llvm/include/llvm/ProfileData/SampleProfReader.h
index f37d7ef798e5..07f0b9ca177d 100644
--- a/llvm/include/llvm/ProfileData/SampleProfReader.h
+++ b/llvm/include/llvm/ProfileData/SampleProfReader.h
@@ -619,8 +619,8 @@ protected:
   sampleprof_error readString(StringRef &Result);
 
   /// Read the string index and check whether it overflows the table.
-  template <typename T> sampleprof_error readStringIndex(T &Table,
-                                   size_t &Result);
+  template <typename T>
+  sampleprof_error readStringIndex(T &Table, size_t &Result);
 
   /// Read the next function profile instance.
   sampleprof_error readFuncProfile(const uint8_t *Start);
@@ -727,9 +727,9 @@ public:
 class SampleProfileReaderExtBinaryBase : public SampleProfileReaderBinary {
 private:
   sampleprof_error decompressSection(const uint8_t *SecStart,
-                                    const uint64_t SecSize,
-                                    const uint8_t *&DecompressBuf,
-                                    uint64_t &DecompressBufSize);
+                                     const uint64_t SecSize,
+                                     const uint8_t *&DecompressBuf,
+                                     uint64_t &DecompressBufSize);
 
   BumpPtrAllocator Allocator;
 
diff --git a/llvm/lib/ProfileData/SampleProfReader.cpp b/llvm/lib/ProfileData/SampleProfReader.cpp
index af2ca10cbb06..f15316172bd4 100644
--- a/llvm/lib/ProfileData/SampleProfReader.cpp
+++ b/llvm/lib/ProfileData/SampleProfReader.cpp
@@ -538,7 +538,7 @@ SampleProfileReaderBinary::readStringFromTable(StringRef &Result,
     assert(MD5NameMemStart);
     using namespace support;
     uint64_t FID = endian::read<uint64_t, little, unaligned>(
-       MD5NameMemStart + (Idx) * sizeof(uint64_t));
+        MD5NameMemStart + (Idx) * sizeof(uint64_t));
     SR = MD5StringBuf.emplace_back(std::to_string(FID));
   }
   if (RetIdx)
@@ -674,7 +674,8 @@ SampleProfileReaderBinary::readProfile(FunctionSamples &FProfile) {
   return sampleprof_error::success;
 }
 
-sampleprof_error SampleProfileReaderBinary::readFuncProfile(const uint8_t *Start) {
+sampleprof_error
+SampleProfileReaderBinary::readFuncProfile(const uint8_t *Start) {
   Data = Start;
   uint64_t NumHeadSamples;
   if (sampleprof_error E = readNumber(NumHeadSamples))
@@ -854,24 +855,24 @@ sampleprof_error SampleProfileReaderExtBinaryBase::readFuncOffsetTable() {
       // Because Porfiles replace existing value with new value if collision
       // happens, we also use the latest offset so that they are consistent.
       FuncOffsetTable[Hash] = Offset;
- }
+  }
 
  return sampleprof_error::success;
 }
 
 sampleprof_error SampleProfileReaderExtBinaryBase::readFuncProfiles() {
-  // Collect functions used by current module if the Reader has been
-  // given a module.
-  // collectFuncsFromModule uses FunctionSamples::getCanonicalFnName
-  // which will query FunctionSamples::HasUniqSuffix, so it has to be
-  // called after FunctionSamples::HasUniqSuffix is set, i.e. after
-  // NameTable section is read.
-  bool LoadFuncsToBeUsed = collectFuncsFromModule();
-
-  // When LoadFuncsToBeUsed is false, we are using LLVM tool, need to read all
-  // profiles.
-  const uint8_t *Start = Data;
-  if (!LoadFuncsToBeUsed) {
+ // Collect functions used by current module if the Reader has been
+ // given a module.
+ // collectFuncsFromModule uses FunctionSamples::getCanonicalFnName
+ // which will query FunctionSamples::HasUniqSuffix, so it has to be
+ // called after FunctionSamples::HasUniqSuffix is set, i.e. after
+ // NameTable section is read.
+ bool LoadFuncsToBeUsed = collectFuncsFromModule();
+
+ // When LoadFuncsToBeUsed is false, we are using LLVM tool, need to read all
+ // profiles.
+ const uint8_t *Start = Data;
+ if (!LoadFuncsToBeUsed) {
     while (Data < End) {
       if (sampleprof_error E = readFuncProfile(Data))
         return E;
@@ -970,7 +971,7 @@ sampleprof_error SampleProfileReaderExtBinaryBase::readProfileSymbolList() {
     ProfSymList = std::make_unique<ProfileSymbolList>();
 
   if (std::error_code EC = ProfSymList->read(Data, End - Data))
-    return (sampleprof_error) EC.value();
+    return (sampleprof_error)EC.value();
 
   Data = End;
   return sampleprof_error::success;
@@ -1103,7 +1104,8 @@ sampleprof_error SampleProfileReaderBinary::readNameTable() {
   return sampleprof_error::success;
 }
 
-sampleprof_error SampleProfileReaderExtBinaryBase::readNameTableSec(bool IsMD5,
+sampleprof_error
+SampleProfileReaderExtBinaryBase::readNameTableSec(bool IsMD5,
                                                    bool FixedLengthMD5) {
   if (FixedLengthMD5) {
     if (!IsMD5)
@@ -1206,7 +1208,8 @@ sampleprof_error SampleProfileReaderExtBinaryBase::readCSNameTableSec() {
   return sampleprof_error::success;
 }
 
-sampleprof_error SampleProfileReaderExtBinaryBase::readFuncMetadata(bool ProfileHasAttribute,
+sampleprof_error
+SampleProfileReaderExtBinaryBase::readFuncMetadata(bool ProfileHasAttribute,
                                                    FunctionSamples *FProfile) {
   if (Data < End) {
     if (ProfileIsProbeBased) {
@@ -1249,8 +1252,7 @@ sampleprof_error SampleProfileReaderExtBinaryBase::readFuncMetadata(bool Profile
         if (FProfile) {
           CalleeProfile = const_cast<FunctionSamples *>(
               &FProfile->functionSamplesAt(LineLocation(
-                  LineOffset,
-                  Discriminator))[std::string(FContext.getName())]);
+                  LineOffset, Discriminator))[std::string(FContext.getName())]);
         }
         if (sampleprof_error E =
                 readFuncMetadata(ProfileHasAttribute, CalleeProfile))
@@ -1285,7 +1287,8 @@ SampleProfileReaderExtBinaryBase::readFuncMetadata(bool ProfileHasAttribute) {
 sampleprof_error
 SampleProfileReaderExtBinaryBase::readSecHdrTableEntry(uint64_t Idx) {
   SecHdrTableEntry Entry;
-  uint64_t Type;;
+  uint64_t Type;
+  ;
   if (sampleprof_error E = readUnencodedNumber(Type))
     return E;
   Entry.Type = static_cast<SecType>(Type);
@@ -1300,7 +1303,8 @@ SampleProfileReaderExtBinaryBase::readSecHdrTableEntry(uint64_t Idx) {
     return E;
   Entry.Offset = Offset;
 
-  uint64_t Size;;
+  uint64_t Size;
+  ;
   if (sampleprof_error E = readUnencodedNumber(Size))
     return E;
   Entry.Size = Size;

[htyu]

I only see changes to the function declaration but not int he function implementation. Am I missing anything or it'll be included in a separate PR?

I also wonder how much end-to-end performance improvement did you see in your experiments? The ErrorOr usage is quite handy and prevailing in the code base.

[huangjd]

The implementation is in the cpp file. This is how the original code is like, since it is a protected function, as long as some other functions in the cpp file instantiate the template, the code will compile fine

[huangjd]

I will do some more benchmarking to see the impact

[huangjd]

Here is comparison of the profile. The left one is baseline and right one is this patch. Showing the disassembly of readNumber because it is the function that is called by almost every other functions in the reader.

[david-xl]

ErrorOr/std:error_code are expensive to construct, so should be limited to error path only. LGTM