[PATCH] D92621: [SampleFDO] Store fixed length MD5 in NameTable instead of using ULEB128 if MD5 is used.

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

> We find that after this change, the elapsed time to build a large application distributively is reduced by 5% and the accumulative cpu time used for building is also reduced by 5%.

This is great, thanks for the patch! Just to clarify, did you mean 5% of end to end ThinLTO+AutoFDO build? I'm curious what is the total profile reading time your saw in terms of %? and how different that % is between md5 profile vs non-md5 profile? Currently we mostly use non-md5 profile, assuming md5 profile is more about saving profile size.



================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:380
+    SR = MD5StringBuf->back();
+    Data = SavedData;
+  }
----------------
If we return error code before reaching this point, `Data` pointer will not be restored, is that ok? What about using RAII to make sure restoration always happens? 
I also noticed this is not the only place where we do save/restore for `Data` pointer, but other instances are not part of this change.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:372
+  // read NameTable index.
+  auto Idx = readMD5StringIndex();
+  if (std::error_code EC = Idx.getError())
----------------
wmi wrote:
> hoy wrote:
> > Can this be just `readStringIndex(NameTable)`  since `NameTable` is preallocated?
> Good point. That makes the code simpler. 
Actually it looks like both fixed path and ULEB128 path need to access `NameTable[*Idx]` in addition to `readStringIndex(NameTable)`? What about just call `SampleProfileReaderBinary::readStringFromTable` for both, and diverge only after that - return for ULEB128 path, check error or empty StringRef for fixed path?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D92621