[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:
> > 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.
Make the comment clearer that MD5Names serves as string buffer that  is referenced by NameTable (vector of StringRef).

Is the lifetime of this buffer guarenteed to be longer than NameTable?


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:733
+    MD5Names->push_back(std::to_string(*FID));
+    NameTable.push_back(MD5Names->back());
+  }
----------------
wmi wrote:
> davidxl wrote:
> > The string is pushed into MD5Names, and immediately copied to NameTable. Can MD5Names be skipped here?
> Ok, I will change it. 
Add a comment here NameTable is a vector of StringRef, and the second push_back is not a copy but a refernece. Otherwise the code looks confusing.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D76255