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

[huangjd]

Summary

Cleanup profile reader classes to prepare for complex refactoring as propsed in D147740 (Use MD5 as key for profile map). Change is too complicated so I am cleaning up the reader implementation first with these goals.

• Reduce duplicated/similar logic
• Reduce virtual functions, changing them to non-virtual
• Reduce unnecessry checks, indirections, and dead writes.

This is patch 1/n. This patch refactors NameTable

Explaining several decisions here

1. useMD5() means whether names of the profiles (the ProfileMap) are represented as MD5. It is NOT whether the input profile format is MD5. This function is an interface for IPO passes to decide whether to match function names or function MD5. There are two motives here:

(a) Eventually we want to use MD5 to represent all function contexts because it is much faster to use it as a key for lookup tables (prototype implementation D147740), so in compilation mode we call setProfileUseMD5() to force use MD5. While in tools mode (llvm-profdata) we want to keep the function name info if it's in the input profile.
(b) We also propose to allow multiple name tables and profile sections in ExtBinary format, and it could consist of name tables with or without using MD5, in this case MD5 prevails and other name tables are converted to MD5.

2. MD5 handling logic is pushed up to BinaryReader base class, because this trades a non-devirtualized virtual function call with a predictable branch. ReadStringFromTable() accounts for >5% time when loading a full 1 GB profile, it should not be virtual.

[davidxl]

llvm/include/llvm/ProfileData/SampleProfReader.h

500

Is there a need to make it virtual? Even though text format does not support it, it is harmless to keep the method (it does nothing).

590

no need for this override.

llvm/lib/ProfileData/SampleProfReader.cpp

538

Can you explain the cleanup done here?

[huangjd]

llvm/include/llvm/ProfileData/SampleProfReader.h

500

Yes this is needed (for now, until MD5 refactoring patch is in place). If this is set, the context uses MD5 as function name, and for the case with Binary format or ExtBinary with function names, these names are converted to MD5 upon read. ProfileIsMD5 is referred by FunctionSamples::UseMD5, which is referred by IPO passes for profile matching. The transform pass needs to know if the profiles are Actually in MD5 because it behaves differently.

llvm/lib/ProfileData/SampleProfReader.cpp

538

This corresponds to

Data = MD5NameMemStart + ((*Idx) * sizeof(uint64_t));
End = reinterpret_cast<const uint8_t *>(
        std::numeric_limits<uintptr_t>::max());
auto FID = readUnencodedNumber<uint64_t>();
if (std::error_code EC = FID.getError())
  return EC;

However the check is not necessary because when reading the name table we already ensured the name table contains correct number of entries, and readStringIndex in this function ensures the index is in range, so the fixed length MD5 can be directly accessed with an index into the base address

[huangjd]

Check endianess when loading fixed length MD5

[huangjd]

llvm/lib/ProfileData/SampleProfReader.cpp

538

Modified code to use endian::read because it is necessary for correctness on big endian platform

[huangjd]

Remove unnecessary cast

[wenlei]

Hoisting all the MD5 stuff out of compact binary format, and merge them with extended binary format makes sense. Initially compact binary format was the only one using MD5, but later on Wei added MD5 support in extended binary as well, which caused some of the duplication this patch is trying to address.

However, from design POV, it's a good practice to keep the specific implementation down to leaf type as much as possible. Here MD5 only comes into play for extended binary and compact binary, so exposing that to all binary format is less than ideal. I think the problem is that compact binary and extended binary started out to be very different, but involved into something similar, so the type structure no longer reflects that commonality. Now in order to deduplicate code, you have to expose MD5 stuff to their lowest common ancestor, which is binary format.

Actually in https://reviews.llvm.org/D76255, we talked about eventually remove compact binary. Can we simply remove compact binary now? Then we can keep MD5 stuff in extended binary still, and there will be no duplicates.

cc @hoy @wlei

llvm/include/llvm/ProfileData/SampleProfReader.h

571

typo: Conexts->Contexts, for->or

[huangjd]

In D148868#4297862, @wenlei wrote:

Hoisting all the MD5 stuff out of compact binary format, and merge them with extended binary format makes sense. Initially compact binary format was the only one using MD5, but later on Wei added MD5 support in extended binary as well, which caused some of the duplication this patch is trying to address.

However, from design POV, it's a good practice to keep the specific implementation down to leaf type as much as possible. Here MD5 only comes into play for extended binary and compact binary, so exposing that to all binary format is less than ideal. I think the problem is that compact binary and extended binary started out to be very different, but involved into something similar, so the type structure no longer reflects that commonality. Now in order to deduplicate code, you have to expose MD5 stuff to their lowest common ancestor, which is binary format.

Actually in https://reviews.llvm.org/D76255, we talked about eventually remove compact binary. Can we simply remove compact binary now? Then we can keep MD5 stuff in extended binary still, and there will be no duplicates.

cc @hoy @wlei

I would like to limit the scope of the refactoring not to include another major change, since other reviewers would like to go through small patches. The purpose of my series of refactoring is to change the data representation of profile map, using MD5 as the key, which could bring significant speedup to profile load time. Reducing overloaded functions with similar logic make the code less error prone when changing the data representation.

TL;DR Eventually, based on external settings (whether we are using ProfileData in compiler or tools), we want the ability to choose whether to store names as MD5, regardless if the actual name table from the file is using Strings, MD5, or fixed length MD5, so the logic should be unified.

[huangjd]

typo

llvm/include/llvm/ProfileData/SampleProfReader.h

571

fixed

[wenlei]

In D148868#4300107, @huangjd wrote:

In D148868#4297862, @wenlei wrote:

Hoisting all the MD5 stuff out of compact binary format, and merge them with extended binary format makes sense. Initially compact binary format was the only one using MD5, but later on Wei added MD5 support in extended binary as well, which caused some of the duplication this patch is trying to address.

However, from design POV, it's a good practice to keep the specific implementation down to leaf type as much as possible. Here MD5 only comes into play for extended binary and compact binary, so exposing that to all binary format is less than ideal. I think the problem is that compact binary and extended binary started out to be very different, but involved into something similar, so the type structure no longer reflects that commonality. Now in order to deduplicate code, you have to expose MD5 stuff to their lowest common ancestor, which is binary format.

Actually in https://reviews.llvm.org/D76255, we talked about eventually remove compact binary. Can we simply remove compact binary now? Then we can keep MD5 stuff in extended binary still, and there will be no duplicates.

cc @hoy @wlei

I would like to limit the scope of the refactoring not to include another major change, since other reviewers would like to go through small patches. The purpose of my series of refactoring is to change the data representation of profile map, using MD5 as the key, which could bring significant speedup to profile load time. Reducing overloaded functions with similar logic make the code less error prone when changing the data representation.

Sure, limit changes to NFC only is good. But the problem is you are exposing MD5 stuff higher up in the type hierarchy to types that shouldn't need to be aware of MD5 details - this created a weird structure. The real problem is the out of sync type structure between compact binary and extended binary. I think that removing compact binary will help this refactoring and also avoid weird structure.

In terms of change size, you can split up a change just to remove compact binary support, then go back to the refactoring. If the goal is to clean things up, it's better to remove obsolete stuff first.

TL;DR Eventually, based on external settings (whether we are using ProfileData in compiler or tools), we want the ability to choose whether to store names as MD5, regardless if the actual name table from the file is using Strings, MD5, or fixed length MD5, so the logic should be unified.

Yeah, and compact binary doesn't have that flexibility. Compact binary become deprecated/obsolete the moment we introduce MD5 to extended binary. We wanted wait for some time before removing it. Now 3 years went by and it's time, and it sort of is getting in the way of this refactoring.

[davidxl]

In D148868#4300595, @wenlei wrote:

In D148868#4300107, @huangjd wrote:

In D148868#4297862, @wenlei wrote:

Hoisting all the MD5 stuff out of compact binary format, and merge them with extended binary format makes sense. Initially compact binary format was the only one using MD5, but later on Wei added MD5 support in extended binary as well, which caused some of the duplication this patch is trying to address.

However, from design POV, it's a good practice to keep the specific implementation down to leaf type as much as possible. Here MD5 only comes into play for extended binary and compact binary, so exposing that to all binary format is less than ideal. I think the problem is that compact binary and extended binary started out to be very different, but involved into something similar, so the type structure no longer reflects that commonality. Now in order to deduplicate code, you have to expose MD5 stuff to their lowest common ancestor, which is binary format.

Actually in https://reviews.llvm.org/D76255, we talked about eventually remove compact binary. Can we simply remove compact binary now? Then we can keep MD5 stuff in extended binary still, and there will be no duplicates.

cc @hoy @wlei

I would like to limit the scope of the refactoring not to include another major change, since other reviewers would like to go through small patches. The purpose of my series of refactoring is to change the data representation of profile map, using MD5 as the key, which could bring significant speedup to profile load time. Reducing overloaded functions with similar logic make the code less error prone when changing the data representation.

Sure, limit changes to NFC only is good. But the problem is you are exposing MD5 stuff higher up in the type hierarchy to types that shouldn't need to be aware of MD5 details - this created a weird structure. The real problem is the out of sync type structure between compact binary and extended binary. I think that removing compact binary will help this refactoring and also avoid weird structure.

In terms of change size, you can split up a change just to remove compact binary support, then go back to the refactoring. If the goal is to clean things up, it's better to remove obsolete stuff first.

TL;DR Eventually, based on external settings (whether we are using ProfileData in compiler or tools), we want the ability to choose whether to store names as MD5, regardless if the actual name table from the file is using Strings, MD5, or fixed length MD5, so the logic should be unified.

Yeah, and compact binary doesn't have that flexibility. Compact binary become deprecated/obsolete the moment we introduce MD5 to extended binary. We wanted wait for some time before removing it. Now 3 years went by and it's time, and it sort of is getting in the way of this refactoring.

Deprecating Compact format seems like a reasonable preparation step for these set of patches (if it does not add too much complexity). My understanding is that it will even simplify the task?

The deprecation itself also helps cleanup LLVM code base. Also users depending on compact format have a migration path (via llvm-profdata) I assume.

[huangjd]

Pushing the MD5 name handling logic up to the base class trades a virtual function call (doesn't seem like being de-virtualized) with a branch, and does lead to a very slight speed improvement on our benchmarking suite.

[huangjd]

Rebase/updated code after removing compact binary

[davidxl]

llvm/lib/ProfileData/SampleProfReader.cpp

539

Add a comment here referencing readUnencodedNumber and mention bounds check is not needed.

717

Is this flag still used or can be asserted to be true?

[huangjd]

update comment on ReadStringFromTable

[huangjd]

llvm/lib/ProfileData/SampleProfReader.cpp

717

It cannot be assumed true. User can specify 3 modes normally:

1. function name strings : ~SecFlagMD5Name && ~SecFlagFixedLengthMD5
2. ULEB128 MD5 : SecFlagMD5Name && ~SecFlagFixedLengthMD5
3. Fixed length MD5 : SecFlagMD5Name && SecFlagFixedLengthMD5

A malformed profile can specify ~SecFlagMD5Name && SecFlagFixedLengthMD5 and crash on the third added test case. I fixed the logic so that LLVM treats this case same as (3).

[davidxl]

llvm/lib/ProfileData/SampleProfReader.cpp

717

is 3 the common and the most efficient one? What is the default with extbinary format? If 3 is the default, we should consider deprecate settings.

[huangjd]

llvm/lib/ProfileData/SampleProfReader.cpp

717

It is not default. The default is Binary format, which I propose to switch to ExtBinary first.

[huangjd]

• Added const qualifier to useMD5()

[davidxl]

llvm/lib/ProfileData/SampleProfReader.cpp

717

Looking forward to the followup cleanup of binary format and related code.

1038

Add assert(IsMD5).

1080

add assert (!FixedLengthMD5);

[huangjd]

• Add assert

[huangjd]

llvm/lib/ProfileData/SampleProfReader.cpp

1038

Added

1080

This is not needed because the previous if(FixedLengthMD5) always returns, so this assert is always true.

[davidxl]

llvm/lib/ProfileData/SampleProfReader.cpp

1080

This is not needed because the previous if(FixedLengthMD5) always returns, so this assert is always true.

right it is redundant with the current control flow. It is probably still worth adding one to prevent problems in the future if the code structure changes.

[huangjd]

• Add assert

[davidxl]

lgtm. Perhaps also add hoy@ or wenlei@ or wlei@ for a quick look.

[huangjd]

• Relaxed assert into a warning instead. FixedLengthMD5 & ~UseMD5 should

llvm/include/llvm/ProfileData/SampleProfReader.h

public:
   };
 
   /// It includes all the names that have samples either in outline instance
   /// or inline instance.
   virtual std::vector<StringRef> *getNameTable() { return nullptr; }
   virtual bool dumpSectionInfo(raw_ostream &OS = dbgs()) { return false; };
 
   /// Return whether names in the profile are all MD5 numbers.
-  virtual bool useMD5() { return false; }
+  bool useMD5() const { return ProfileIsMD5; }
+
+  /// Force the profile to use MD5 in Sample contexts, even if function names
+  /// are present.
+  virtual void setProfileUseMD5() { ProfileIsMD5 = true; }
 
   /// Don't read profile without context if the flag is set. This is only meaningful
   /// for ExtBinary format.
   virtual void setSkipFlatProf(bool Skip) {}
   /// Return whether any name in the profile contains ".__uniq." suffix.
   virtual bool hasUniqSuffix() { return false; }
 
   SampleProfileReaderItaniumRemapper *getRemapper() { return Remapper.get(); }
protected:
   /// \brief The current module being compiled if SampleProfileReader
   /// is used by compiler. If SampleProfileReader is used by other
   /// tools which are not compiler, M is usually nullptr.
   const Module *M = nullptr;
 
   /// Zero out the discriminator bits higher than bit MaskedBitFrom (0 based).
   /// The default is to keep all the bits.
   uint32_t MaskedBitFrom = 31;
+
+  /// Whether the profile uses MD5 for Sample Contexts and function names. This
+  /// can be one-way overriden by the user to force use MD5.
+  bool ProfileIsMD5 = false;
 };
 
 class SampleProfileReaderText : public SampleProfileReader {
 public:
   SampleProfileReaderText(std::unique_ptr<MemoryBuffer> B, LLVMContext &C)
       : SampleProfileReader(std::move(B), C, SPF_Text) {}
 
   /// Read and validate the file header.
   std::error_code readHeader() override { return sampleprof_error::success; }
 
   /// Read sample profiles from the associated file.
   std::error_code readImpl() override;
 
   /// Return true if \p Buffer is in the format supported by this class.
   static bool hasFormat(const MemoryBuffer &Buffer);
 
+  /// Text format sample profile does not support MD5 for now.
+  void setProfileUseMD5() override {}
+
 private:
   /// CSNameTable is used to save full context vectors. This serves as an
   /// underlying immutable buffer for all clients.
   std::list<SampleContextFrameVector> CSNameTable;
 };
 
 class SampleProfileReaderBinary : public SampleProfileReader {
 public:
protected:
 
   /// Read the contents of Magic number and Version number.
   std::error_code readMagicIdent();
 
   /// Read profile summary.
   std::error_code readSummary();
 
   /// Read the whole name table.
-  virtual std::error_code readNameTable();
+  std::error_code readNameTable();
+
+  /// Read a string indirectly via the name table.
+  ErrorOr<StringRef> readStringFromTable();
 
   /// 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;
 
-  /// Read a string indirectly via the name table.
-  virtual ErrorOr<StringRef> readStringFromTable();
+  /// 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();
 
 private:
   std::error_code readSummaryEntry(std::vector<ProfileSummaryEntry> &Entries);
   virtual std::error_code verifySPMagic(uint64_t Magic) = 0;
 };
 
 class SampleProfileReaderRawBinary : public SampleProfileReaderBinary {
protected:
   std::error_code readSecHdrTableEntry(uint64_t Idx);
   std::error_code readSecHdrTable();
 
   std::error_code readFuncMetadata(bool ProfileHasAttribute);
   std::error_code readFuncMetadata(bool ProfileHasAttribute,
                                    FunctionSamples *FProfile);
   std::error_code readFuncOffsetTable();
   std::error_code readFuncProfiles();
-  std::error_code readMD5NameTable();
-  std::error_code readNameTableSec(bool IsMD5);
+  std::error_code readNameTableSec(bool IsMD5, bool FixedLengthMD5);
   std::error_code readCSNameTableSec();
   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<StringRef> readStringFromTable() override;
   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;
 
-  /// Use fixed length MD5 instead of ULEB128 encoding so NameTable doesn't
-  /// need to be read in up front and can be directly accessed using index.
-  bool FixedLengthMD5 = false;
-  /// The starting address of NameTable containing fixed length MD5.
-  const uint8_t *MD5NameMemStart = nullptr;
-
-  /// 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::unique_ptr<std::vector<std::string>> MD5StringBuf;
-
   /// 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;
 
public:
   /// Get the total size of header and all sections.
   uint64_t getFileSize();
   bool dumpSectionInfo(raw_ostream &OS = dbgs()) override;
 
   /// Collect functions with definitions in Module M. Return true if
   /// the reader has been given a module.
   bool collectFuncsFromModule() override;
 
-  /// Return whether names in the profile are all MD5 numbers.
-  bool useMD5() override { return MD5StringBuf.get(); }
-
   std::unique_ptr<ProfileSymbolList> getProfileSymbolList() override {
     return std::move(ProfSymList);
   };
 
   void setSkipFlatProf(bool Skip) override { SkipFlatProf = Skip; }
 };
 
 class SampleProfileReaderExtBinary : public SampleProfileReaderExtBinaryBase {

llvm/lib/ProfileData/SampleProfReader.cpp

inline ErrorOr<size_t> SampleProfileReaderBinary::readStringIndex(T &Table) {
   return *Idx;
 }
 
 ErrorOr<StringRef> SampleProfileReaderBinary::readStringFromTable() {
   auto Idx = readStringIndex(NameTable);
   if (std::error_code EC = Idx.getError())
     return EC;
 
-  return NameTable[*Idx];
+  // Lazy loading, if the string has not been materialized from memory storing
+  // MD5 values, then it is default initialized with the null pointer. This can
+  // only happen when using fixed length MD5, that bounds check is performed
+  // while parsing the name table to ensure MD5NameMemStart points to an array
+  // with enough MD5 entries.
+  StringRef &SR = NameTable[*Idx];
+  if (!SR.data()) {
+    assert(MD5NameMemStart);
+    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<SampleContext> SampleProfileReaderBinary::readSampleContextFromTable() {
   auto FName(readStringFromTable());
   if (std::error_code EC = FName.getError())
     return EC;
   return SampleContext(*FName);
 }
 
-ErrorOr<StringRef> SampleProfileReaderExtBinaryBase::readStringFromTable() {
-  if (!FixedLengthMD5)
-    return SampleProfileReaderBinary::readStringFromTable();
-
-  // read NameTable index.
-  auto Idx = readStringIndex(NameTable);
-  if (std::error_code EC = Idx.getError())
-    return EC;
-
-  // Check whether the name to be accessed has been accessed before,
-  // if not, read it from memory directly.
-  StringRef &SR = NameTable[*Idx];
-  if (SR.empty()) {
-    const uint8_t *SavedData = Data;
-    Data = MD5NameMemStart + ((*Idx) * sizeof(uint64_t));
-    auto FID = readUnencodedNumber<uint64_t>();
-    if (std::error_code EC = FID.getError())
-      return EC;
-    // Save the string converted from uint64_t in MD5StringBuf. All the
-    // references to the name are all StringRefs refering to the string
-    // in MD5StringBuf.
-    MD5StringBuf->push_back(std::to_string(*FID));
-    SR = MD5StringBuf->back();
-    Data = SavedData;
-  }
-  return SR;
-}
-
 std::error_code
 SampleProfileReaderBinary::readProfile(FunctionSamples &FProfile) {
   auto NumSamples = readNumber<uint64_t>();
   if (std::error_code EC = NumSamples.getError())
     return EC;
   FProfile.addTotalSamples(*NumSamples);
 
   // Read the samples in the body.
case SecProfSummary:
     if (hasSecFlag(Entry, SecProfSummaryFlags::SecFlagFullContext))
       FunctionSamples::ProfileIsCS = ProfileIsCS = true;
     if (hasSecFlag(Entry, SecProfSummaryFlags::SecFlagIsPreInlined))
       FunctionSamples::ProfileIsPreInlined = ProfileIsPreInlined = true;
     if (hasSecFlag(Entry, SecProfSummaryFlags::SecFlagFSDiscriminator))
       FunctionSamples::ProfileIsFS = ProfileIsFS = true;
     break;
   case SecNameTable: {
-    FixedLengthMD5 =
+    bool FixedLengthMD5 =
         hasSecFlag(Entry, SecNameTableFlags::SecFlagFixedLengthMD5);
     bool UseMD5 = hasSecFlag(Entry, SecNameTableFlags::SecFlagMD5Name);
-    assert((!FixedLengthMD5 || UseMD5) &&
-           "If FixedLengthMD5 is true, UseMD5 has to be true");
+    // UseMD5 means if THIS section uses MD5, ProfileIsMD5 means if the entire
+    // profile uses MD5 for function name matching in IPO passes.
+    ProfileIsMD5 = ProfileIsMD5 || UseMD5;
     FunctionSamples::HasUniqSuffix =
         hasSecFlag(Entry, SecNameTableFlags::SecFlagUniqSuffix);
-    if (std::error_code EC = readNameTableSec(UseMD5))
+    if (std::error_code EC = readNameTableSec(UseMD5, FixedLengthMD5))
       return EC;
     break;
   }
   case SecCSNameTable: {
     if (std::error_code EC = readCSNameTableSec())
       return EC;
     break;
   }
if (Magic == SPMagic(SPF_Ext_Binary))
     return sampleprof_error::success;
   return sampleprof_error::bad_magic;
 }
 
 std::error_code SampleProfileReaderBinary::readNameTable() {
   auto Size = readNumber<size_t>();
   if (std::error_code EC = Size.getError())
     return EC;
-  NameTable.reserve(*Size + NameTable.size());
+
+  // Normally if useMD5 is true, the name table should have MD5 values, not
+  // strings, however in the case that ExtBinary profile has multiple name
+  // tables mixing string and MD5, all of them have to be normalized to use MD5,
+  // because optimization passes can only handle either type.
+  bool UseMD5 = useMD5();
+  if (UseMD5)
+    MD5StringBuf.reserve(MD5StringBuf.size() + *Size);
+
+  NameTable.clear();
+  NameTable.reserve(*Size);
   for (size_t I = 0; I < *Size; ++I) {
     auto Name(readString());
     if (std::error_code EC = Name.getError())
       return EC;
+    if (UseMD5) {
+      uint64_t FID = MD5Hash(*Name);
+      NameTable.emplace_back(MD5StringBuf.emplace_back(std::to_string(FID)));
+    } else
       NameTable.push_back(*Name);
   }
 
   return sampleprof_error::success;
 }
 
-std::error_code SampleProfileReaderExtBinaryBase::readMD5NameTable() {
-  auto Size = readNumber<uint64_t>();
+std::error_code
+SampleProfileReaderExtBinaryBase::readNameTableSec(bool IsMD5,
+                                                   bool FixedLengthMD5) {
+  if (FixedLengthMD5) {
+    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;
-  MD5StringBuf = std::make_unique<std::vector<std::string>>();
-  MD5StringBuf->reserve(*Size);
-  if (FixedLengthMD5) {
+
+    assert(Data + (*Size) * sizeof(uint64_t) == End &&
+           "Fixed length MD5 name table does not contain specified number of "
+           "entries");
+    if (Data + (*Size) * sizeof(uint64_t) > End)
+      return sampleprof_error::truncated;
+
     // Preallocate and initialize NameTable so we can check whether a name
     // index has been read before by checking whether the element in the
     // NameTable is empty, meanwhile readStringIndex can do the boundary
     // check using the size of NameTable.
-    NameTable.resize(*Size + NameTable.size());
-
+    MD5StringBuf.reserve(MD5StringBuf.size() + *Size);
+    NameTable.clear();
+    NameTable.resize(*Size);
     MD5NameMemStart = Data;
     Data = Data + (*Size) * sizeof(uint64_t);
     return sampleprof_error::success;
   }
+
+  if (IsMD5) {
+    assert(!FixedLengthMD5 && "FixedLengthMD5 should be unreachable here");
+    auto Size = readNumber<size_t>();
+    if (std::error_code EC = Size.getError())
+      return EC;
+
+    MD5StringBuf.reserve(MD5StringBuf.size() + *Size);
+    NameTable.clear();
     NameTable.reserve(*Size);
-  for (uint64_t I = 0; I < *Size; ++I) {
+    for (size_t I = 0; I < *Size; ++I) {
       auto FID = readNumber<uint64_t>();
       if (std::error_code EC = FID.getError())
         return EC;
-    MD5StringBuf->push_back(std::to_string(*FID));
-    // NameTable is a vector of StringRef. Here it is pushing back a
-    // StringRef initialized with the last string in MD5stringBuf.
-    NameTable.push_back(MD5StringBuf->back());
+      NameTable.emplace_back(MD5StringBuf.emplace_back(std::to_string(*FID)));
     }
     return sampleprof_error::success;
   }
 
-std::error_code SampleProfileReaderExtBinaryBase::readNameTableSec(bool IsMD5) {
-  if (IsMD5)
-    return readMD5NameTable();
   return SampleProfileReaderBinary::readNameTable();
 }
 
 // Read in the CS name table section, which basically contains a list of context
 // 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>();

llvm/test/tools/llvm-profdata/sample-nametable.test

+Test several edge cases with unusual name table data in ExtBinary format.
+
+1- Multiple fixed-length MD5 name tables. Reading a new table should clear the content from old table, and a valid name index for the old name table should become invalid if the new name table has fewer entries.
+RUN: not llvm-profdata show --sample %p/Inputs/sample-multiple-nametables.profdata
+
+2- Multiple name tables, the first one has an empty string, the second one tricks the reader into expecting fixed-length MD5 values. Reader should not attempt "lazy loading" of the MD5 string in this case.
+RUN: not llvm-profdata show --sample %p/Inputs/sample-nametable-empty-string.profdata
+
+3- The data of the name table is placed after the data of the profiles. The reader should handle it correctly.
+RUN: llvm-profdata merge --sample --text %p/Inputs/sample-nametable-after-samples.profdata | FileCheck %s
+CHECK: 18446744073709551615:2:9
+