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

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish marked 5 inline comments as done.
snehasish added a comment.

Thanks for the review, ptal!



================
Comment at: llvm/include/llvm/ProfileData/MemProf.h:231
+    size_t Result = 0;
+    Result ^= U64Hasher(Function) + 0x9e3779b9 + (Result << 6) + (Result >> 2);
+    Result ^=
----------------
tejohnson wrote:
> 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.
This is inspired by boost::hash_combine. :) 
Updated the code a bit here to reduce the verbosity, updated the constant to a 64-bit value (since at least one element is 64-bit) and added a comment to make things clear. There is some more info about the constant here: https://softwareengineering.stackexchange.com/a/402543


================
Comment at: llvm/unittests/ProfileData/InstrProfTest.cpp:260
+
+  auto Equals = [&]() {
+    if (Want.AllocSites.size() != Got.AllocSites.size())
----------------
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.


================
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();
+
----------------
tejohnson wrote:
> Should this commented code line be here?
Leftover from a prior iteration, removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123094