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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

lgtm, couple minor things noted below.



================
Comment at: llvm/include/llvm/ProfileData/InstrProfWriter.h:48
+      MemProfRecordData;
+  // A map to hold memprof id to frame mappings. The mappings are used to
+  // convert IndexedMemProfRecord to MemProfRecords with frame information
----------------
s/memprof id/frame id/ ?


================
Comment at: llvm/unittests/ProfileData/InstrProfTest.cpp:272
+
+  if (Want.AllocSites.size() != Got.AllocSites.size()) {
+    return PrintAndFail();
----------------
Nit, remove the braces around single line bodies here and below.


================
Comment at: llvm/unittests/ProfileData/InstrProfTest.cpp:260
+
+  auto Equals = [&]() {
+    if (Want.AllocSites.size() != Got.AllocSites.size())
----------------
snehasish wrote:
> tejohnson wrote:
> > Not clear to me why a lambda is needed here since it is only invoked once directly below. Is it just for readability?
> Using the Equals lambda in the prior diff allowed me to have a single place for the printing code. In this diff, I've refactored out the print code as a lambda and inlined the comparison. I don't have a strong preference for either, let me know what you think.
Ah ok, that makes sense. I like the way you have it organized in the new version, it is a little clearer.


================
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();
+
----------------
snehasish wrote:
> tejohnson wrote:
> > Should this commented code line be here?
> Leftover from a prior iteration, removed.
Is the comment relevant here or should that be removed too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123094