[llvm-profdata] ProfileReader cleanup - preparation for MD5 refactoring - 2

[huangjd]

Summary

Cleanup profile reader classes to prepare for complex refactoring as propsed in D147740, continuing D148868
This is patch 2/n. This patch refactors CSNameTable and related things

The decision to move CSNameTable up to the base class is because a planned improvement (D147740) to use MD5 to lookup Functions/Context frames. In this case we want a unified data structure between contextless function or Context frames, so that it can be mapped by MD5 value. Since Context Frames can represent contextless functions, it is being used for MD5 lookup, therefore exposing it to the base class

[davidxl]

Looks like a good cleanup. CC wenlei and hoy for a closer look.

[hoy]

llvm/include/llvm/ProfileData/SampleProfReader.h

690

Are we planning to retire SampleProfileReaderCompactBinary? The functions and fields being refactored are not supposed to be available for that class.

[wenlei]

Cleanup profile reader classes to prepare for complex refactoring as propsed in D147740, continuing D148868

Might be a bit of nitpicking, but thought I'd mentioned it since it's related to earlier comment in another patch (D148876) about explaining "why" in change description - here I'd appreciate if you can explain why you need "CSNameTable and related things" to be moved up in the type hierarchy, and how that helps to prepare for the change to improve FDO build speed.

My understanding is that since you need to use MD5 as key for the profile map, everything related to MD5, names, CSnames need to be exposed as the top level interface. It's useful to others if you articulate the why explicitly upfront and the same goes for D148868.

llvm/include/llvm/ProfileData/SampleProfReader.h

690

Well... one more reason to remove compact binary support first before this refactoring. As pointed out in the other patch, compact binary is effectively deprecated, and new features are not supported with compact binary. So it creates a dilemma with refactoring as deduplication by hosting would move things that don't actually apply to compact binary to its parent class.

[huangjd]

Rebase after compact binary deprecated

[snehasish]

LGTM, please wait a bit in case others have any concerns.

llvm/lib/ProfileData/SampleProfReader.cpp

564

Can we drop the else (and indentation for the block below) since this part is unconditional after the early return?

[wenlei]

lgtm, thanks.

[huangjd]

Rebase

llvm/include/llvm/ProfileData/SampleProfReader.h

protected:
   std::error_code readSummary();
 
   /// Read the whole name table.
   std::error_code readNameTable();
 
   /// Read a string indirectly via the name table.
   ErrorOr<StringRef> readStringFromTable();
 
+  /// Read a context indirectly via the CSNameTable.
+  ErrorOr<SampleContextFrames> readContextFromTable();
+
+  /// Read a context indirectly via the CSNameTable if the profile has context,
+  /// otherwise same as readStringFromTable.
+  ErrorOr<SampleContext> readSampleContextFromTable();
+
   /// Points to the current location in the buffer.
   const uint8_t *Data = nullptr;
 
   /// Points to the end of the buffer.
   const uint8_t *End = nullptr;
 
   /// Function name table.
   std::vector<StringRef> NameTable;
 
   /// If MD5 is used in NameTable section, the section saves uint64_t data.
   /// The uint64_t data has to be converted to a string and then the string
   /// will be used to initialize StringRef in NameTable.
   /// Note NameTable contains StringRef so it needs another buffer to own
   /// the string data. MD5StringBuf serves as the string buffer that is
   /// referenced by NameTable (vector of StringRef). We make sure
   /// the lifetime of MD5StringBuf is not shorter than that of NameTable.
   std::vector<std::string> MD5StringBuf;
 
   /// The starting address of NameTable containing fixed length MD5.
   const uint8_t *MD5NameMemStart = nullptr;
 
-  virtual ErrorOr<SampleContext> readSampleContextFromTable();
+  /// CSNameTable is used to save full context vectors. It is the backing buffer
+  /// for SampleContextFrames.
+  std::vector<SampleContextFrameVector> CSNameTable;
 
 private:
   std::error_code readSummaryEntry(std::vector<ProfileSummaryEntry> &Entries);
   virtual std::error_code verifySPMagic(uint64_t Magic) = 0;
 };
 
 class SampleProfileReaderRawBinary : public SampleProfileReaderBinary {
 private:
protected:
   std::error_code readProfileSymbolList();
 
   std::error_code readHeader() override;
   std::error_code verifySPMagic(uint64_t Magic) override = 0;
   virtual std::error_code readOneSection(const uint8_t *Start, uint64_t Size,
                                          const SecHdrTableEntry &Entry);
   // placeholder for subclasses to dispatch their own section readers.
   virtual std::error_code readCustomSection(const SecHdrTableEntry &Entry) = 0;
-  ErrorOr<SampleContext> readSampleContextFromTable() override;
-  ErrorOr<SampleContextFrames> readContextFromTable();
 
   std::unique_ptr<ProfileSymbolList> ProfSymList;
 
   /// The table mapping from function context to the offset of its
   /// FunctionSample towards file start.
   DenseMap<SampleContext, uint64_t> FuncOffsetTable;
 
   /// Function offset mapping ordered by contexts.
   std::unique_ptr<std::vector<std::pair<SampleContext, uint64_t>>>
       OrderedFuncOffsets;
 
   /// The set containing the functions to use when compiling a module.
   DenseSet<StringRef> FuncsToUse;
 
-  /// CSNameTable is used to save full context vectors. This serves as an
-  /// underlying immutable buffer for all clients.
-  std::unique_ptr<const std::vector<SampleContextFrameVector>> CSNameTable;
-
   /// If SkipFlatProf is true, skip the sections with
   /// SecFlagFlat flag.
   bool SkipFlatProf = false;
 
   bool FuncOffsetsOrdered = false;
 
 public:
   SampleProfileReaderExtBinaryBase(std::unique_ptr<MemoryBuffer> B,

llvm/lib/ProfileData/SampleProfReader.cpp

if (!SR.data()) {
     using namespace support;
     uint64_t FID = endian::read<uint64_t, little, unaligned>(
        MD5NameMemStart + (*Idx) * sizeof(uint64_t));
     SR = MD5StringBuf.emplace_back(std::to_string(FID));
   }
   return SR;
 }
 
+ErrorOr<SampleContextFrames> SampleProfileReaderBinary::readContextFromTable() {
+  auto ContextIdx = readNumber<size_t>();
+  if (std::error_code EC = ContextIdx.getError())
+    return EC;
+  if (*ContextIdx >= CSNameTable.size())
+    return sampleprof_error::truncated_name_table;
+  return CSNameTable[*ContextIdx];
+}
+
 ErrorOr<SampleContext> SampleProfileReaderBinary::readSampleContextFromTable() {
+  if (ProfileIsCS) {
+    auto FContext(readContextFromTable());
+    if (std::error_code EC = FContext.getError())
+      return EC;
+    return SampleContext(*FContext);
+  } else {
     auto FName(readStringFromTable());
     if (std::error_code EC = FName.getError())
       return EC;
     return SampleContext(*FName);
   }
+}
 
 std::error_code
 SampleProfileReaderBinary::readProfile(FunctionSamples &FProfile) {
   auto NumSamples = readNumber<uint64_t>();
   if (std::error_code EC = NumSamples.getError())
     return EC;
   FProfile.addTotalSamples(*NumSamples);
 
std::error_code SampleProfileReaderBinary::readImpl() {
   while (!at_eof()) {
     if (std::error_code EC = readFuncProfile(Data))
       return EC;
   }
 
   return sampleprof_error::success;
 }
 
-ErrorOr<SampleContextFrames>
-SampleProfileReaderExtBinaryBase::readContextFromTable() {
-  auto ContextIdx = readNumber<size_t>();
-  if (std::error_code EC = ContextIdx.getError())
-    return EC;
-  if (*ContextIdx >= CSNameTable->size())
-    return sampleprof_error::truncated_name_table;
-  return (*CSNameTable)[*ContextIdx];
-}
-
-ErrorOr<SampleContext>
-SampleProfileReaderExtBinaryBase::readSampleContextFromTable() {
-  if (ProfileIsCS) {
-    auto FContext(readContextFromTable());
-    if (std::error_code EC = FContext.getError())
-      return EC;
-    return SampleContext(*FContext);
-  } else {
-    auto FName(readStringFromTable());
-    if (std::error_code EC = FName.getError())
-      return EC;
-    return SampleContext(*FName);
-  }
-}
-
 std::error_code SampleProfileReaderExtBinaryBase::readOneSection(
     const uint8_t *Start, uint64_t Size, const SecHdrTableEntry &Entry) {
   Data = Start;
   End = Start + Size;
   switch (Entry.Type) {
   case SecProfSummary:
     if (std::error_code EC = readSummary())
       return EC;
std::error_code SampleProfileReaderBinary::readNameTable() {
 
   return sampleprof_error::success;
 }
 
 std::error_code
 SampleProfileReaderExtBinaryBase::readNameTableSec(bool IsMD5,
                                                    bool FixedLengthMD5) {
   if (FixedLengthMD5) {
-    if (IsMD5)
+    if (!IsMD5)
       errs() << "If FixedLengthMD5 is true, UseMD5 has to be true";
     auto Size = readNumber<size_t>();
     if (std::error_code EC = Size.getError())
       return EC;
 
     assert(Data + (*Size) * sizeof(uint64_t) == End &&
            "Fixed length MD5 name table does not contain specified number of "
            "entries");

 // vectors. Each element of a context vector, aka a frame, refers to the
 // underlying raw function names that are stored in the name table, as well as
 // a callsite identifier that only makes sense for non-leaf frames.
 std::error_code SampleProfileReaderExtBinaryBase::readCSNameTableSec() {
   auto Size = readNumber<size_t>();
   if (std::error_code EC = Size.getError())
     return EC;
 
-  std::vector<SampleContextFrameVector> *PNameVec =
-      new std::vector<SampleContextFrameVector>();
-  PNameVec->reserve(*Size);
+  CSNameTable.clear();
+  CSNameTable.reserve(*Size);
   for (size_t I = 0; I < *Size; ++I) {
-    PNameVec->emplace_back(SampleContextFrameVector());
+    CSNameTable.emplace_back(SampleContextFrameVector());
     auto ContextSize = readNumber<uint32_t>();
     if (std::error_code EC = ContextSize.getError())
       return EC;
     for (uint32_t J = 0; J < *ContextSize; ++J) {
       auto FName(readStringFromTable());
       if (std::error_code EC = FName.getError())
         return EC;
       auto LineOffset = readNumber<uint64_t>();
       if (std::error_code EC = LineOffset.getError())
         return EC;
 
       if (!isOffsetLegal(*LineOffset))
         return std::error_code();
 
       auto Discriminator = readNumber<uint64_t>();
       if (std::error_code EC = Discriminator.getError())
         return EC;
 
-      PNameVec->back().emplace_back(
+      CSNameTable.back().emplace_back(
           FName.get(), LineLocation(LineOffset.get(), Discriminator.get()));
     }
   }
 
-  // From this point the underlying object of CSNameTable should be immutable.
-  CSNameTable.reset(PNameVec);
   return sampleprof_error::success;
 }
 
 std::error_code
-
 SampleProfileReaderExtBinaryBase::readFuncMetadata(bool ProfileHasAttribute,
                                                    FunctionSamples *FProfile) {
   if (Data < End) {
     if (ProfileIsProbeBased) {
       auto Checksum = readNumber<uint64_t>();
       if (std::error_code EC = Checksum.getError())
         return EC;
       if (FProfile)