[PATCH] D68601: [SampleFDO] Add indexing for function profiles so they can be loaded on demand in ExtBinary format

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

Thanks for making indexing available for extended binary format. We've recently adding indexing to binary format as well in an internal patch, and also observed similar (~30%) build time reduction for some large services.

We also have have the need to differentiate dead/cold symbols vs new symbols, so symbol list in extended binary format is useful to us as well - will try it out. (I noticed that a small change in profile generation tool (the equivalent of https://github.com/google/autofdo) is needed to populate that list though, it'd be nice if these are all part of LLVM)

Left some comments inline. In addition, I think `llvm-profdata/Inputs/sample-profile.proftext` need to be updated as well to include the new section for `roundtrip.test`



================
Comment at: llvm/include/llvm/ProfileData/SampleProfWriter.h:206
   virtual void initSectionLayout() override {
     SectionLayout = {{SecProfSummary, 0, 0, 0},
                      {SecNameTable, 0, 0, 0},
----------------
With the addition of offset table at the end of sections but in the middle of section header, I found the name SectionLayout a bit confusing. This array actually represents the layout/order of section header (e.g. the reader order), but not the section (payload) layout (the writer order). I guess renaming it SectionHdrLayout or something alike may help..

Besides, some comments explaining why SecFuncOffsetTable need to be in the middle could help too, it looks like a hidden trick until I saw the comments in SampleProfileReaderExtBinaryBase::getFileSize().


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:547-548
+      continue;
+    Data = Start + iter->second;
+    if (std::error_code EC = readFuncProfile())
+      return EC;
----------------
nit: similar to line 539, we could assert the random access here never attempts to touch anything beyond Size? 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68601