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

[Wei Mi via Phabricator via llvm-commits]

wmi marked 3 inline comments as done.
wmi 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;
+
----------------
davidxl wrote:
> 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?
There is another one defined as vector<StringRef> in the base class SampleProfileReaderBinary.


================
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
----------------
davidxl wrote:
> NameTable Decl here
Yes, Compbinary format uses std::vector<std::string> type of NameTable. When we use std::vector<std::string> for NameTable, we can avoid the buffer, but we are going to add more override functions. 

Currently I used the buffer in the patch, but it is confusing that we used two different ways in Compbinary format and in ExtBinary format for md5 support. So maybe using std::vector<std::string> type for NameTable is better. I will change it. 


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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D76255