[PATCH] D76255: [SampleFDO] Port MD5 name table support to extbinary format.

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProfReader.h:637
+  /// converted from uint64_t data.
+  std::unique_ptr<std::vector<std::string>> MD5Names;
+
----------------
wmi wrote:
> davidxl wrote:
> > What is the use of MD5Names?  It seems only used in useMD5() method.
> > 
> > Also why does it need to be unique_ptr?
> Profile only contains md5 numbers (type of uint64_t). NameTable contains StringRef to the names (All formats of profile use the same NameTable). When reading profile using md5, to populate the NameTable structure, I need to convert uint64_t number to std::string and then convert it to StringRef, so I need a buffer to save those std::string because StringRef doesn't own any string data. To be a buffer of NameTable, that is the use of MD5Names. 
> 
> I make it a unique_ptr because for formats not using md5, MD5Names is useless, so I think an empty unique_ptr may have less cost than an empty std::vector in that case. 
The type of NameTable is vector<string> not vector<StringRef> right?


================
Comment at: llvm/include/llvm/ProfileData/SampleProfReader.h:664
   /// Function name table.
   std::vector<std::string> NameTable;
   /// The table mapping from function name to the offset of its FunctionSample
----------------
NameTable Decl here


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:733
+    MD5Names->push_back(std::to_string(*FID));
+    NameTable.push_back(MD5Names->back());
+  }
----------------
The string is pushed into MD5Names, and immediately copied to NameTable. Can MD5Names be skipped here?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D76255