[PATCH] D147740: [NFC][llvm-profdata] Refactoring Sample Profile Reader to increase FDO build speed

[William Junda Huang via Phabricator via llvm-commits]

huangjd added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:635
+    // consistent with the profile map's key.
+    // If non-context function is already MD5 string, do not hash again.
+    uint64_t Hash;
----------------
davidxl wrote:
> how often is the name already MD5 string?
If the profile is stored as MD5 format, then all of them are MD5 string, otherwise none. 

This is some logic from before the refactoring, that the MD5 function name is being converted back and forth between a uint64 and string, and I am planning to change it to a union or similar in the next phase 


================
Comment at: llvm/include/llvm/ProfileData/SampleProfReader.h:520
   /// This should only be needed for md5 profiles.
-  std::unordered_set<std::string> MD5NameBuffer;
+  std::list<std::string> MD5NameBuffer;
 
----------------
davidxl wrote:
> why changing it to list?
It's faster. The buffer is never directly accessed. It is a backing buffer for StringRefs.


================
Comment at: llvm/include/llvm/ProfileData/SampleProfReader.h:658
+
+  /// Read the whole name table consists of ULEB128 encoded MD5 values.
+  std::error_code readMD5NameTable();
----------------
davidxl wrote:
> How effective is MD5 compacting with ULEB128 ? Why not use fixed length rep to save some computation time?
This is from existing design, a profile can store MD5 as fixed length or ULEB128. The implementation contains logic to support both 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147740