[memprof] Introduce IndexedCallstackIdConveter (NFC) #120540
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch introduces IndexedCallstackIdConveter as a convenience
wrapper around FrameIdConverter and CallStackIdConverter just for
tests.
With the new wrapper, we get to replace idioms like:
FrameIdConverter<decltype(MemProfData.Frames)> FrameIdConv(
MemProfData.Frames);
CallStackIdConverter<decltype(MemProfData.CallStacks)> CSIdConv(
MemProfData.CallStacks, FrameIdConv);
with:
IndexedCallstackIdConveter CSIdConv(MemProfData);
Unfortunately, this exact pattern occurs in tests only; the
combinations of the frame ID converter and call stack ID converter are
diverse in production code.
|
@llvm/pr-subscribers-pgo Author: Kazu Hirata (kazutakahirata) ChangesThis patch introduces IndexedCallstackIdConveter as a convenience With the new wrapper, we get to replace idioms like: FrameIdConverter<decltype(MemProfData.Frames)> FrameIdConv( with: IndexedCallstackIdConveter CSIdConv(MemProfData); Unfortunately, this exact pattern occurs in tests only; the Full diff: https://github.com/llvm/llvm-project/pull/120540.diff 3 Files Affected:
diff --git a/llvm/include/llvm/ProfileData/MemProf.h b/llvm/include/llvm/ProfileData/MemProf.h
index 883e24d7186152..da0cb47508e32b 100644
--- a/llvm/include/llvm/ProfileData/MemProf.h
+++ b/llvm/include/llvm/ProfileData/MemProf.h
@@ -1030,6 +1030,27 @@ struct IndexedMemProfData {
CallStackId hashCallStack(ArrayRef<FrameId> CS) const;
};
+// A convenience wrapper around FrameIdConverter and CallStackIdConverter for
+// tests.
+struct IndexedCallstackIdConveter {
+ IndexedCallstackIdConveter() = delete;
+ IndexedCallstackIdConveter(IndexedMemProfData &MemProfData)
+ : FrameIdConv(MemProfData.Frames),
+ CSIdConv(MemProfData.CallStacks, FrameIdConv) {}
+
+ // Delete the copy constructor and copy assignment operator to avoid a
+ // situation where a copy of IndexedCallStackIdConverter gets an error in
+ // LastUnmappedId while the original instance doesn't.
+ IndexedCallstackIdConveter(const IndexedCallstackIdConveter &) = delete;
+ IndexedCallstackIdConveter &
+ operator=(const IndexedCallstackIdConveter &) = delete;
+
+ std::vector<Frame> operator()(CallStackId CSId) { return CSIdConv(CSId); }
+
+ FrameIdConverter<decltype(IndexedMemProfData::Frames)> FrameIdConv;
+ CallStackIdConverter<decltype(IndexedMemProfData::CallStacks)> CSIdConv;
+};
+
struct FrameStat {
// The number of occurrences of a given FrameId.
uint64_t Count = 0;
diff --git a/llvm/unittests/ProfileData/InstrProfTest.cpp b/llvm/unittests/ProfileData/InstrProfTest.cpp
index ac872d8235622b..adcd4d2d11e0f1 100644
--- a/llvm/unittests/ProfileData/InstrProfTest.cpp
+++ b/llvm/unittests/ProfileData/InstrProfTest.cpp
@@ -457,17 +457,14 @@ TEST_F(InstrProfTest, test_memprof_v2_full_schema) {
ASSERT_THAT_ERROR(RecordOr.takeError(), Succeeded());
const memprof::MemProfRecord &Record = RecordOr.get();
- memprof::FrameIdConverter<decltype(MemProfData.Frames)> FrameIdConv(
- MemProfData.Frames);
- memprof::CallStackIdConverter<decltype(MemProfData.CallStacks)> CSIdConv(
- MemProfData.CallStacks, FrameIdConv);
+ memprof::IndexedCallstackIdConveter CSIdConv(MemProfData);
const ::llvm::memprof::MemProfRecord WantRecord =
IndexedMR.toMemProfRecord(CSIdConv);
- ASSERT_EQ(FrameIdConv.LastUnmappedId, std::nullopt)
- << "could not map frame id: " << *FrameIdConv.LastUnmappedId;
- ASSERT_EQ(CSIdConv.LastUnmappedId, std::nullopt)
- << "could not map call stack id: " << *CSIdConv.LastUnmappedId;
+ ASSERT_EQ(CSIdConv.FrameIdConv.LastUnmappedId, std::nullopt)
+ << "could not map frame id: " << *CSIdConv.FrameIdConv.LastUnmappedId;
+ ASSERT_EQ(CSIdConv.CSIdConv.LastUnmappedId, std::nullopt)
+ << "could not map call stack id: " << *CSIdConv.CSIdConv.LastUnmappedId;
EXPECT_THAT(WantRecord, EqualsRecord(Record));
}
@@ -494,17 +491,14 @@ TEST_F(InstrProfTest, test_memprof_v2_partial_schema) {
ASSERT_THAT_ERROR(RecordOr.takeError(), Succeeded());
const memprof::MemProfRecord &Record = RecordOr.get();
- memprof::FrameIdConverter<decltype(MemProfData.Frames)> FrameIdConv(
- MemProfData.Frames);
- memprof::CallStackIdConverter<decltype(MemProfData.CallStacks)> CSIdConv(
- MemProfData.CallStacks, FrameIdConv);
+ memprof::IndexedCallstackIdConveter CSIdConv(MemProfData);
const ::llvm::memprof::MemProfRecord WantRecord =
IndexedMR.toMemProfRecord(CSIdConv);
- ASSERT_EQ(FrameIdConv.LastUnmappedId, std::nullopt)
- << "could not map frame id: " << *FrameIdConv.LastUnmappedId;
- ASSERT_EQ(CSIdConv.LastUnmappedId, std::nullopt)
- << "could not map call stack id: " << *CSIdConv.LastUnmappedId;
+ ASSERT_EQ(CSIdConv.FrameIdConv.LastUnmappedId, std::nullopt)
+ << "could not map frame id: " << *CSIdConv.FrameIdConv.LastUnmappedId;
+ ASSERT_EQ(CSIdConv.CSIdConv.LastUnmappedId, std::nullopt)
+ << "could not map call stack id: " << *CSIdConv.CSIdConv.LastUnmappedId;
EXPECT_THAT(WantRecord, EqualsRecord(Record));
}
@@ -615,10 +609,7 @@ TEST_F(InstrProfTest, test_memprof_merge) {
std::optional<memprof::FrameId> LastUnmappedFrameId;
- memprof::FrameIdConverter<decltype(MemProfData.Frames)> FrameIdConv(
- MemProfData.Frames);
- memprof::CallStackIdConverter<decltype(MemProfData.CallStacks)> CSIdConv(
- MemProfData.CallStacks, FrameIdConv);
+ memprof::IndexedCallstackIdConveter CSIdConv(MemProfData);
const ::llvm::memprof::MemProfRecord WantRecord =
IndexedMR.toMemProfRecord(CSIdConv);
diff --git a/llvm/unittests/ProfileData/MemProfTest.cpp b/llvm/unittests/ProfileData/MemProfTest.cpp
index 2eb85d5b2f5878..1fe9af521d884c 100644
--- a/llvm/unittests/ProfileData/MemProfTest.cpp
+++ b/llvm/unittests/ProfileData/MemProfTest.cpp
@@ -501,16 +501,13 @@ TEST(MemProf, IndexedMemProfRecordToMemProfRecord) {
IndexedRecord.CallSiteIds.push_back(CS3Id);
IndexedRecord.CallSiteIds.push_back(CS4Id);
- FrameIdConverter<decltype(MemProfData.Frames)> FrameIdConv(
- MemProfData.Frames);
- CallStackIdConverter<decltype(MemProfData.CallStacks)> CSIdConv(
- MemProfData.CallStacks, FrameIdConv);
+ IndexedCallstackIdConveter CSIdConv(MemProfData);
MemProfRecord Record = IndexedRecord.toMemProfRecord(CSIdConv);
// Make sure that all lookups are successful.
- ASSERT_EQ(FrameIdConv.LastUnmappedId, std::nullopt);
- ASSERT_EQ(CSIdConv.LastUnmappedId, std::nullopt);
+ ASSERT_EQ(CSIdConv.FrameIdConv.LastUnmappedId, std::nullopt);
+ ASSERT_EQ(CSIdConv.CSIdConv.LastUnmappedId, std::nullopt);
// Verify the contents of Record.
ASSERT_THAT(Record.AllocSites, SizeIs(2));
@@ -540,17 +537,14 @@ TEST(MemProf, MissingCallStackId) {
// Create empty maps.
IndexedMemProfData MemProfData;
- FrameIdConverter<decltype(MemProfData.Frames)> FrameIdConv(
- MemProfData.Frames);
- CallStackIdConverter<decltype(MemProfData.CallStacks)> CSIdConv(
- MemProfData.CallStacks, FrameIdConv);
+ IndexedCallstackIdConveter CSIdConv(MemProfData);
// We are only interested in errors, not the return value.
(void)IndexedMR.toMemProfRecord(CSIdConv);
- ASSERT_TRUE(CSIdConv.LastUnmappedId.has_value());
- EXPECT_EQ(*CSIdConv.LastUnmappedId, 0xdeadbeefU);
- EXPECT_EQ(FrameIdConv.LastUnmappedId, std::nullopt);
+ ASSERT_TRUE(CSIdConv.CSIdConv.LastUnmappedId.has_value());
+ EXPECT_EQ(*CSIdConv.CSIdConv.LastUnmappedId, 0xdeadbeefU);
+ EXPECT_EQ(CSIdConv.FrameIdConv.LastUnmappedId, std::nullopt);
}
TEST(MemProf, MissingFrameId) {
@@ -561,17 +555,14 @@ TEST(MemProf, MissingFrameId) {
IndexedMemProfRecord IndexedMR;
IndexedMR.AllocSites.emplace_back(CSId, makePartialMIB(), getHotColdSchema());
- FrameIdConverter<decltype(MemProfData.Frames)> FrameIdConv(
- MemProfData.Frames);
- CallStackIdConverter<decltype(MemProfData.CallStacks)> CSIdConv(
- MemProfData.CallStacks, FrameIdConv);
+ IndexedCallstackIdConveter CSIdConv(MemProfData);
// We are only interested in errors, not the return value.
(void)IndexedMR.toMemProfRecord(CSIdConv);
- EXPECT_EQ(CSIdConv.LastUnmappedId, std::nullopt);
- ASSERT_TRUE(FrameIdConv.LastUnmappedId.has_value());
- EXPECT_EQ(*FrameIdConv.LastUnmappedId, 3U);
+ EXPECT_EQ(CSIdConv.CSIdConv.LastUnmappedId, std::nullopt);
+ ASSERT_TRUE(CSIdConv.FrameIdConv.LastUnmappedId.has_value());
+ EXPECT_EQ(*CSIdConv.FrameIdConv.LastUnmappedId, 3U);
}
// Verify CallStackRadixTreeBuilder can handle empty inputs.
@@ -714,10 +705,7 @@ TEST(MemProf, YAMLParser) {
const auto &[GUID, IndexedRecord] = MemProfData.Records.front();
EXPECT_EQ(GUID, 0xdeadbeef12345678ULL);
- FrameIdConverter<decltype(MemProfData.Frames)> FrameIdConv(
- MemProfData.Frames);
- CallStackIdConverter<decltype(MemProfData.CallStacks)> CSIdConv(
- MemProfData.CallStacks, FrameIdConv);
+ IndexedCallstackIdConveter CSIdConv(MemProfData);
MemProfRecord Record = IndexedRecord.toMemProfRecord(CSIdConv);
ASSERT_THAT(Record.AllocSites, SizeIs(2));
@@ -760,10 +748,7 @@ TEST(MemProf, YAMLParserGUID) {
const auto &[GUID, IndexedRecord] = MemProfData.Records.front();
EXPECT_EQ(GUID, IndexedMemProfRecord::getGUID("_Z3fooi"));
- FrameIdConverter<decltype(MemProfData.Frames)> FrameIdConv(
- MemProfData.Frames);
- CallStackIdConverter<decltype(MemProfData.CallStacks)> CSIdConv(
- MemProfData.CallStacks, FrameIdConv);
+ IndexedCallstackIdConveter CSIdConv(MemProfData);
MemProfRecord Record = IndexedRecord.toMemProfRecord(CSIdConv);
ASSERT_THAT(Record.AllocSites, SizeIs(1));
|
teresajohnson
approved these changes
Dec 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch introduces IndexedCallstackIdConveter as a convenience
wrapper around FrameIdConverter and CallStackIdConverter just for
tests.
With the new wrapper, we get to replace idioms like:
FrameIdConverter<decltype(MemProfData.Frames)> FrameIdConv(
MemProfData.Frames);
CallStackIdConverter<decltype(MemProfData.CallStacks)> CSIdConv(
MemProfData.CallStacks, FrameIdConv);
with:
IndexedCallstackIdConveter CSIdConv(MemProfData);
Unfortunately, this exact pattern occurs in tests only; the
combinations of the frame ID converter and call stack ID converter are
diverse in production code.