[PATCH] D123094: [memprof] Deduplicate and outline frame storage in the memprof profile.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Great! Few minor comments/questions.



================
Comment at: llvm/include/llvm/ProfileData/InstrProfWriter.h:48
+      MemProfRecordData;
+  // A map to hold memprof frame to id mappings. The mappings are used to
+  // convert IndexedMemProfRecord to MemProfRecords with frame information
----------------
>From the declaration it looks like it is the reverse: holds memprof frame id to frame mappings.


================
Comment at: llvm/include/llvm/ProfileData/MemProf.h:231
+    size_t Result = 0;
+    Result ^= U64Hasher(Function) + 0x9e3779b9 + (Result << 6) + (Result >> 2);
+    Result ^=
----------------
Out of curiosity, how did you choose the hashing algorithm and the value 0x9e3779b9? Is it worth documenting?

Also, the shifts on Result are unnecessary on this line since it is init to 0 just above, although if you think it is clearer to leave all the lines the same that's fine.


================
Comment at: llvm/lib/ProfileData/InstrProfReader.cpp:972
     const unsigned char *Ptr = Start + MemProfOffset;
-    // The value returned from Generator.Emit.
-    const uint64_t TableOffset =
+    // The value returned from RecordGenerator.Emit.
+    const uint64_t RecordTableOffset =
----------------
RecordTableGenerator?


================
Comment at: llvm/lib/ProfileData/InstrProfReader.cpp:977
+        support::endian::readNext<uint64_t, little, unaligned>(Ptr);
+    // The value returned from FrameGenerator.Emit.
+    const uint64_t FrameTableOffset =
----------------
FrameTableGenerator?


================
Comment at: llvm/lib/ProfileData/InstrProfWriter.cpp:258
+    const Function::GUID Id, const memprof::IndexedMemProfRecord &Record,
+    function_ref<void(Error)> /*Unused*/) {
+  auto Result = MemProfRecordData.insert({Id, Record});
----------------
Why not change the function signature to not take this parameter and then not pass it everywhere? Or do we plan to use this in the future? In the latter case, add a TODO.


================
Comment at: llvm/lib/ProfileData/InstrProfWriter.cpp:408
+  // uint64_t RecordTableOffset = RecordTableGenerator.Emit
+  // uint64_t FrameTableOffset = FrameTableGenerator.Emit
   // uint64_t Num schema entries
----------------
Missing documentation for the FramePayloadOffset emitted above FrameTableOffset?


================
Comment at: llvm/unittests/ProfileData/InstrProfTest.cpp:260
+
+  auto Equals = [&]() {
+    if (Want.AllocSites.size() != Got.AllocSites.size())
----------------
Not clear to me why a lambda is needed here since it is only invoked once directly below. Is it just for readability?


================
Comment at: llvm/unittests/ProfileData/MemProfTest.cpp:179
+  // the mappings to the matcher to extract the frame and match the contents.
+  // const auto &IdToFrame = Reader.getFrameMapping();
+
----------------
Should this commented code line be here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123094