Skip to content

[memprof] Add a new constructor to MemProfReader (NFC) #116918

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

kazutakahirata
Copy link
Contributor

This patch adds a new constructor to MemProfReader that takes
IndexedMemProfData, a complete package of MemProf profile. To
showcase its usage, I'm updating one of the unit tests to use the new
constructor.

Because of type mismatches between DenseMap and MapVector, I'm copying
Frames and CallStacks for now. Once we remove the methods and old
constructors that take or return individual components (frames, call
stacks, and records), we will drop the copying, and the new
constructor will collapse down to:

MemProfReader(IndexedMemProfData MemProfData)
: MemProfData(std::move(MemProfData)) {}

Since nobody in the LLVM codebase uses the constructor that takes the
three indivdual components, I'm deprecating the old constructor.

This patch adds a new constructor to MemProfReader that takes
IndexedMemProfData, a complete package of MemProf profile.  To
showcase its usage, I'm updating one of the unit tests to use the new
constructor.

Because of type mismatches between DenseMap and MapVector, I'm copying
Frames and CallStacks for now.  Once we remove the methods and old
constructors that take or return individual components (frames, call
stacks, and records), we will drop the copying, and the new
constructor will collapse down to:

  MemProfReader(IndexedMemProfData MemProfData)
    : MemProfData(std::move(MemProfData)) {}

Since nobody in the LLVM codebase uses the constructor that takes the
three indivdual components, I'm deprecating the old constructor.
@llvmbot llvmbot added the PGO Profile Guided Optimizations label Nov 20, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 20, 2024

@llvm/pr-subscribers-pgo

Author: Kazu Hirata (kazutakahirata)

Changes

This patch adds a new constructor to MemProfReader that takes
IndexedMemProfData, a complete package of MemProf profile. To
showcase its usage, I'm updating one of the unit tests to use the new
constructor.

Because of type mismatches between DenseMap and MapVector, I'm copying
Frames and CallStacks for now. Once we remove the methods and old
constructors that take or return individual components (frames, call
stacks, and records), we will drop the copying, and the new
constructor will collapse down to:

MemProfReader(IndexedMemProfData MemProfData)
: MemProfData(std::move(MemProfData)) {}

Since nobody in the LLVM codebase uses the constructor that takes the
three indivdual components, I'm deprecating the old constructor.


Full diff: https://github.com/llvm/llvm-project/pull/116918.diff

2 Files Affected:

  • (modified) llvm/include/llvm/ProfileData/MemProfReader.h (+11)
  • (modified) llvm/unittests/ProfileData/MemProfTest.cpp (+6-8)
diff --git a/llvm/include/llvm/ProfileData/MemProfReader.h b/llvm/include/llvm/ProfileData/MemProfReader.h
index de2167f97b0dc8..ade0c3901562d5 100644
--- a/llvm/include/llvm/ProfileData/MemProfReader.h
+++ b/llvm/include/llvm/ProfileData/MemProfReader.h
@@ -120,6 +120,8 @@ class MemProfReader {
 
   // Initialize the MemProfReader with the frame mappings, call stack mappings,
   // and profile contents.
+  LLVM_DEPRECATED("Construct MemProfReader with IndexedMemProfData",
+                  "MemProfReader")
   MemProfReader(
       llvm::DenseMap<FrameId, Frame> FrameIdMap,
       llvm::DenseMap<CallStackId, llvm::SmallVector<FrameId>> CSIdMap,
@@ -127,6 +129,15 @@ class MemProfReader {
       : IdToFrame(std::move(FrameIdMap)), CSIdToCallStack(std::move(CSIdMap)),
         FunctionProfileData(std::move(ProfData)) {}
 
+  // Initialize the MemProfReader with the given MemProf profile.
+  MemProfReader(IndexedMemProfData MemProfData) {
+    for (const auto &[FrameId, F] : MemProfData.Frames)
+      IdToFrame.try_emplace(FrameId, F);
+    for (const auto &[CSId, CS] : MemProfData.CallStacks)
+      CSIdToCallStack.try_emplace(CSId, CS);
+    FunctionProfileData = std::move(MemProfData.Records);
+  }
+
 protected:
   // A helper method to extract the frame from the IdToFrame map.
   const Frame &idToFrame(const FrameId Id) const {
diff --git a/llvm/unittests/ProfileData/MemProfTest.cpp b/llvm/unittests/ProfileData/MemProfTest.cpp
index 5097dbdd6c3915..e5d41db06dcc07 100644
--- a/llvm/unittests/ProfileData/MemProfTest.cpp
+++ b/llvm/unittests/ProfileData/MemProfTest.cpp
@@ -490,20 +490,18 @@ TEST(MemProf, BaseMemProfReader) {
 }
 
 TEST(MemProf, BaseMemProfReaderWithCSIdMap) {
-  llvm::DenseMap<FrameId, Frame> FrameIdMap;
+  llvm::memprof::IndexedMemProfData MemProfData;
   Frame F1(/*Hash=*/IndexedMemProfRecord::getGUID("foo"), /*LineOffset=*/20,
            /*Column=*/5, /*IsInlineFrame=*/true);
   Frame F2(/*Hash=*/IndexedMemProfRecord::getGUID("bar"), /*LineOffset=*/10,
            /*Column=*/2, /*IsInlineFrame=*/false);
-  FrameIdMap.insert({F1.hash(), F1});
-  FrameIdMap.insert({F2.hash(), F2});
+  MemProfData.Frames.insert({F1.hash(), F1});
+  MemProfData.Frames.insert({F2.hash(), F2});
 
-  llvm::DenseMap<CallStackId, llvm::SmallVector<FrameId>> CSIdMap;
   llvm::SmallVector<FrameId> CallStack = {F1.hash(), F2.hash()};
   CallStackId CSId = llvm::memprof::hashCallStack(CallStack);
-  CSIdMap.insert({CSId, CallStack});
+  MemProfData.CallStacks.insert({CSId, CallStack});
 
-  llvm::MapVector<llvm::GlobalValue::GUID, IndexedMemProfRecord> ProfData;
   IndexedMemProfRecord FakeRecord;
   MemInfoBlock Block;
   Block.AllocCount = 1U, Block.TotalAccessDensity = 4,
@@ -511,9 +509,9 @@ TEST(MemProf, BaseMemProfReaderWithCSIdMap) {
   FakeRecord.AllocSites.emplace_back(
       /*CSId=*/llvm::memprof::hashCallStack(CallStack),
       /*MB=*/Block);
-  ProfData.insert({F1.hash(), FakeRecord});
+  MemProfData.Records.insert({F1.hash(), FakeRecord});
 
-  MemProfReader Reader(FrameIdMap, CSIdMap, ProfData);
+  MemProfReader Reader(MemProfData);
 
   llvm::SmallVector<MemProfRecord, 1> Records;
   for (const auto &KeyRecordPair : Reader) {

@kazutakahirata kazutakahirata merged commit f88c913 into llvm:main Nov 20, 2024
8 of 10 checks passed
@kazutakahirata kazutakahirata deleted the memprof_cleanup_MemProfReader_ctor branch November 20, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PGO Profile Guided Optimizations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants