[memprof] Make HeapProfileRecords optional #155671
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
memprof::AllMemProfData has become home to multiple types of memprof data -- heap profile and static data profile. When we write test cases for static data profile in YAML, we do not want to include empty heap profile. That would just add visual clutter. This patch makes HeapProfileRecords optional.
|
@llvm/pr-subscribers-pgo Author: Kazu Hirata (kazutakahirata) Changesmemprof::AllMemProfData has become home to multiple types of memprof This patch makes HeapProfileRecords optional. Full diff: https://github.com/llvm/llvm-project/pull/155671.diff 1 Files Affected:
diff --git a/llvm/include/llvm/ProfileData/MemProfYAML.h b/llvm/include/llvm/ProfileData/MemProfYAML.h
index ad5d7c0e22751..87e2f0ea14487 100644
--- a/llvm/include/llvm/ProfileData/MemProfYAML.h
+++ b/llvm/include/llvm/ProfileData/MemProfYAML.h
@@ -263,7 +263,8 @@ template <> struct MappingTraits<memprof::YamlDataAccessProfData> {
template <> struct MappingTraits<memprof::AllMemProfData> {
static void mapping(IO &Io, memprof::AllMemProfData &Data) {
- Io.mapRequired("HeapProfileRecords", Data.HeapProfileRecords);
+ if (!Io.outputting() || !Data.HeapProfileRecords.empty())
+ Io.mapOptional("HeapProfileRecords", Data.HeapProfileRecords);
// Map data access profiles if reading input, or if writing output &&
// the struct is populated.
if (!Io.outputting() || !Data.YamlifiedDataAccessProfiles.isEmpty())
|
There was a problem hiding this comment.
Right now this section itself requires both AllocSites and CallSites. Does it make sense to change that too? In practice we need not have both in a profile.
Should there be a test for this change to ensure it does something reasonable for an empty memprof yaml?
mingmingl-llvm
approved these changes
Aug 27, 2025
Let me take that as a follow-up project.
Yes, and that turns out to be important. We need to relax the emptiness check in |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/169/builds/14456 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/143/builds/10359 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
memprof::AllMemProfData has become home to multiple types of memprof
data -- heap profile and static data profile. When we write test
cases for static data profile in YAML, we do not want to include empty
heap profile. That would just add visual clutter.
This patch makes HeapProfileRecords optional.