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

[Wei Mi via Phabricator via llvm-commits]

wmi marked an inline comment as done.
wmi added inline comments.


================
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
----------------
wmi wrote:
> 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. 
I just tried then I realized a problem I forgot to mention: When ExtBinary format doesn't use md5, it wants NameTable to be std::vector<StringRef> type. When ExtBinary format uses md5, it wants NameTable to be std::vector<std::string>. I can create a new nametable called MD5NameTable, but it makes things more complicate. 

We also don't want to use std::vector<std::string> type NameTable for both cases. That is because for ExtBinary format without using md5 the string has already existed in file buffer. If NameTable can use StringRef, it can refer to the string in file buffer directly without having to copy the string to NameTable. That will save some cost. 

So looks like keeping the MD5Names buffer is a simpler way to me.     


Repository:
  rL LLVM

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

https://reviews.llvm.org/D76255