search

llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.78k stars 11.9k forks source link

[memprof] Pass FrameIdConverter and CallStackIdConverter by reference #92327

Closed kazutakahirata closed 5 months ago

kazutakahirata commented 5 months ago

CallStackIdConverter sets LastUnmappedId when a mapping failure occurs. Now, since toMemProfRecord takes an instance of CallStackIdConverter by value, namely std::function, the caller of toMemProfRecord never receives the mapping failure that occurs inside toMemProfRecord. The same problem applies to FrameIdConverter.

The patch fixes the problem by passing FrameIdConverter and CallStackIdConverter by reference, namely llvm::function_ref.

While I am it, this patch deletes the copy constructor and copy assignment operator to avoid accidental copies.

llvmbot commented 5 months ago

@llvm/pr-subscribers-pgo

Author: Kazu Hirata (kazutakahirata)

Changes CallStackIdConverter sets LastUnmappedId when a mapping failure occurs. Now, since toMemProfRecord takes an instance of CallStackIdConverter by value, namely std::function, the caller of toMemProfRecord never receives the mapping failure that occurs inside toMemProfRecord. The same problem applies to FrameIdConverter. The patch fixes the problem by passing FrameIdConverter and CallStackIdConverter by reference, namely llvm::function_ref. While I am it, this patch deletes the copy constructor and copy assignment operator to avoid accidental copies. --- Full diff: https://github.com/llvm/llvm-project/pull/92327.diff 3 Files Affected: - (modified) llvm/include/llvm/ProfileData/MemProf.h (+17-4) - (modified) llvm/lib/ProfileData/MemProf.cpp (+2-2) - (modified) llvm/unittests/ProfileData/InstrProfTest.cpp (+46) ``````````diff diff --git a/llvm/include/llvm/ProfileData/MemProf.h b/llvm/include/llvm/ProfileData/MemProf.h index 3ef6ca8586fb2..60bff76d0e464 100644 --- a/llvm/include/llvm/ProfileData/MemProf.h +++ b/llvm/include/llvm/ProfileData/MemProf.h @@ -426,8 +426,8 @@ struct IndexedMemProfRecord { // Convert IndexedMemProfRecord to MemProfRecord. Callback is used to // translate CallStackId to call stacks with frames inline. MemProfRecord toMemProfRecord( - std::function(const CallStackId)> Callback) - const; + llvm::function_ref(const CallStackId)> + Callback) const; // Returns the GUID for the function name after canonicalization. For // memprof, we remove any .llvm suffix added by LTO. MemProfRecords are @@ -784,6 +784,12 @@ template struct FrameIdConverter { FrameIdConverter() = delete; FrameIdConverter(MapTy &Map) : Map(Map) {} + // Delete the copy constructor and copy assignment operator to avoid a + // situation where a copy of FrameIdConverter gets an error in LastUnmappedId + // while the original instance doesn't. + FrameIdConverter(const FrameIdConverter &) = delete; + FrameIdConverter &operator=(const FrameIdConverter &) = delete; + Frame operator()(FrameId Id) { auto Iter = Map.find(Id); if (Iter == Map.end()) { @@ -798,12 +804,19 @@ template struct FrameIdConverter { template struct CallStackIdConverter { std::optional LastUnmappedId; MapTy ⤅ - std::function FrameIdToFrame; + llvm::function_ref FrameIdToFrame; CallStackIdConverter() = delete; - CallStackIdConverter(MapTy &Map, std::function FrameIdToFrame) + CallStackIdConverter(MapTy &Map, + llvm::function_ref FrameIdToFrame) : Map(Map), FrameIdToFrame(FrameIdToFrame) {} + // Delete the copy constructor and copy assignment operator to avoid a + // situation where a copy of CallStackIdConverter gets an error in + // LastUnmappedId while the original instance doesn't. + CallStackIdConverter(const CallStackIdConverter &) = delete; + CallStackIdConverter &operator=(const CallStackIdConverter &) = delete; + llvm::SmallVector operator()(CallStackId CSId) { llvm::SmallVector Frames; auto CSIter = Map.find(CSId); diff --git a/llvm/lib/ProfileData/MemProf.cpp b/llvm/lib/ProfileData/MemProf.cpp index 4667778ca11dd..f5789186094c6 100644 --- a/llvm/lib/ProfileData/MemProf.cpp +++ b/llvm/lib/ProfileData/MemProf.cpp @@ -243,8 +243,8 @@ IndexedMemProfRecord::deserialize(const MemProfSchema &Schema, } MemProfRecord IndexedMemProfRecord::toMemProfRecord( - std::function(const CallStackId)> Callback) - const { + llvm::function_ref(const CallStackId)> + Callback) const { MemProfRecord Record; for (const memprof::IndexedAllocationInfo &IndexedAI : AllocSites) { diff --git a/llvm/unittests/ProfileData/InstrProfTest.cpp b/llvm/unittests/ProfileData/InstrProfTest.cpp index 924d848176e77..33ff49b1806ac 100644 --- a/llvm/unittests/ProfileData/InstrProfTest.cpp +++ b/llvm/unittests/ProfileData/InstrProfTest.cpp @@ -581,6 +581,52 @@ TEST_F(InstrProfTest, test_memprof_v2_partial_schema) { EXPECT_THAT(WantRecord, EqualsRecord(Record)); } +TEST_F(InstrProfTest, test_memprof_missing_callstackid) { + // Use a non-existent CallStackId to trigger a mapping error in + // toMemProfRecord. + llvm::memprof::IndexedAllocationInfo AI({}, 0xdeadbeefU, makePartialMIB(), + llvm::memprof::getHotColdSchema()); + + IndexedMemProfRecord IndexedMR; + IndexedMR.AllocSites.push_back(AI); + + const FrameIdMapTy IdToFrameMap = getFrameMapping(); + const auto CSIdToCallStackMap = getCallStackMapping(); + memprof::FrameIdConverter FrameIdConv(IdToFrameMap); + memprof::CallStackIdConverter CSIdConv( + CSIdToCallStackMap, FrameIdConv); + + // 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); +} + +TEST_F(InstrProfTest, test_memprof_missing_frameid) { + llvm::memprof::IndexedAllocationInfo AI({}, 0x222, makePartialMIB(), + llvm::memprof::getHotColdSchema()); + + IndexedMemProfRecord IndexedMR; + IndexedMR.AllocSites.push_back(AI); + + // An empty map to trigger a mapping error. + const FrameIdMapTy IdToFrameMap; + const auto CSIdToCallStackMap = getCallStackMapping(); + + memprof::FrameIdConverter FrameIdConv(IdToFrameMap); + memprof::CallStackIdConverter CSIdConv( + CSIdToCallStackMap, FrameIdConv); + + // 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); +} + TEST_F(InstrProfTest, test_memprof_getrecord_error) { ASSERT_THAT_ERROR(Writer.mergeProfileKind(InstrProfKind::MemProf), Succeeded()); ``````````