[PATCH] D108435: [CSSPGO] split context string II - reader/writer changes

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProfWriter.h:138
   std::error_code writeSummary();
-  std::error_code writeNameIdx(StringRef FName, bool IsContextName = false);
+  virtual std::error_code writeNameIdx(const SampleContext &Context);
+  std::error_code writeNameIdx(StringRef FName);
----------------
Now that the parameter is no longer a string name, rename the function as well, e.g. writeContextIdx? 

Same for addName(const SampleContext &Context) -> addContext


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:829-846
       for (auto Name : FuncOffsetTable) {
         OrderedNames.insert(Name.first);
       }
 
       // For each function in current module, load all
       // context profiles for the function.
       for (auto NameOffset : FuncOffsetTable) {
----------------
Which operation here is the most costly and visible for e2e build time? The insert/sort/find or the prefix check? What's the % of time here for both prelink and postlink?


================
Comment at: llvm/lib/ProfileData/SampleProfWriter.cpp:249
+  for (auto Context : OrderedContexts) {
+    auto Callsites = Context.getContextFrames();
+    encodeULEB128(Callsites.size(), OS);
----------------
nit: this contains leaf frame, so it's `Frames` instead of `Callsites`. 


================
Comment at: llvm/lib/ProfileData/SampleProfWriter.cpp:482
+SampleProfileWriterBinary::writeNameIdx(const SampleContext &Context) {
+  return writeNameIdx(Context.getName());
+}
----------------
assert !Context.hasContext() ?


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:524
   for (const auto &PD : Reader->getProfiles()) {
-    StringRef FName = PD.getKey();
-    const sampleprof::FunctionSamples &FS = PD.getValue();
-    auto It = InstrProfileMap.find(FName);
+    auto &FName = PD.first;
+    const sampleprof::FunctionSamples &FS = PD.second;
----------------
I suggest rename these FName (key of the SampleProfileMap) to be FContext accordingly given this is no longer a string map. Same for other places.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108435