[PATCH] D121179: [memprof] Store callsite metadata with memprof records.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/MemProf.h:237
+  llvm::SmallVector<AllocationInfo> AllocSites;
+  llvm::SmallVector<llvm::SmallVector<Frame>> CallSites;
 
----------------
It would be good to document the data structure choice - i.e. why this is a vector of vectors


================
Comment at: llvm/include/llvm/ProfileData/MemProf.h:286
 
   bool operator==(const MemProfRecord &Other) const {
+    if (Other.AllocSites.size() != AllocSites.size())
----------------
Should this be comparing CallSites as well?

Also, it might be nice to refactor some of the code below into an operator== in the AllocationInfo struct.


================
Comment at: llvm/include/llvm/ProfileData/RawMemProfReader.h:103
   Error symbolizeAndFilterStackFrames();
+  // Construct memprof records for each function that and store it in the
+  // `FunctionProfileData` map. A function may have allocation profile data or
----------------
missing word(s) after "that"?


================
Comment at: llvm/lib/ProfileData/RawMemProfReader.cpp:301
+  // keep track of related contexts so that we can fill these in later.
+  llvm::DenseMap<Function::GUID, llvm::SetVector<uint64_t>>
+      PerFunctionCallSites;
----------------
Document what the uint64_t values are.


================
Comment at: llvm/lib/ProfileData/RawMemProfReader.cpp:324
+
+      // The leaf function is the first PC in the callstack so check if unset.
+      if (LeafFunction == 0) {
----------------
Why can't the leaf function be an inline frame? I.e. what if we have:

foo() { malloc(); }
bar() { foo(); }

and we inline foo into bar.

Won't the leaf be foo which will be an inline frame?


================
Comment at: llvm/lib/ProfileData/RawMemProfReader.cpp:332
+        // the leaf since that call is to the allocator itself.
+        PerFunctionCallSites[Frames.front().Function].insert(Address);
+      }
----------------
If we want to avoid a hash table lookup to SymbolizedFrame for each of these addresses in the later loop, we could store a pointer to the the DenseMap iterator value_type for SymbolizedFrame[Address].


================
Comment at: llvm/test/tools/llvm-profdata/memprof-basic.test:50
+CHECK-NEXT:     FunctionGUID: {{[0-9]+}} 
+CHECK-NEXT:     AllocSites:
 CHECK-NEXT:     -
----------------
Can this test now also look for the CallSites?


================
Comment at: llvm/unittests/ProfileData/InstrProfTest.cpp:228
+using ::llvm::memprof::MemProfRecord;
+MemProfRecord makeRecord(std::initializer_list<MemProfRecord::Frame> Frames,
+                         const MemInfoBlock &Block = MemInfoBlock()) {
----------------
Should this file also be testing CallSites?


================
Comment at: llvm/unittests/ProfileData/InstrProfTest.cpp:283
 
-TEST_F(InstrProfTest, test_memprof_invalid_add_record) {
-  llvm::memprof::MemProfRecord MR;
-  // At least one of the frames should be a non-inline frame.
-  MR.CallStack.push_back({0x123, 1, 2, true});
-  MR.CallStack.push_back({0x345, 3, 4, true});
-
-  auto CheckErr = [](Error &&E) {
-    EXPECT_TRUE(ErrorEquals(instrprof_error::invalid_prof, std::move(E)));
-  };
-  Writer.addRecord(MR, CheckErr);
-}
+// TEST_F(InstrProfTest, test_memprof_invalid_add_record) {
+//   llvm::memprof::MemProfRecord MR;
----------------
Why is this test commented out now?


================
Comment at: llvm/unittests/ProfileData/MemProfTest.cpp:189
+  EXPECT_THAT(Records[0].AllocSites[0].CallStack[1],
+              FrameContains("bar", 51U, 20U, true));
+
----------------
Should this callstack now have 4 entries and can all be checked?

Additionally, should the record for foo now have CallSites since it shows up in the call stack for the allocation in CSM[0x2]?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121179