[MemProf] Streamline and avoid unnecessary context id duplication #107918
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
Sort the list of calls such that those with the same stack ids are also sorted by function. This allows processing of all matching calls (that can share a context node) in bulk as they are all adjacent. This has 2 benefits: 1. It reduces unnecessary work, specifically the handling to intersect the context ids with those along the graph edges for the stack ids, for calls that we know can share a node. 2. It simplifies detecting when we have matching stack ids but don't need to duplicate context ids. Specifically, we were previously still duplicating context ids whenever we saw another call with the same stack ids, but that isn't necessary if they will share a context node. With this change we now only duplicate context ids if we see some that not only have the same ids but also are in different functions. This change reduced the amount of context id duplication and provided reductions in both both peak memory (~8%) and time (~%5) for a large target.
|
@llvm/pr-subscribers-llvm-transforms Author: Teresa Johnson (teresajohnson) ChangesSort the list of calls such that those with the same stack ids are also This has 2 benefits:
This change reduced the amount of context id duplication and provided Full diff: https://github.com/llvm/llvm-project/pull/107918.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp b/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp
index f8c150a675e645..5c09bb1800cb2b 100644
--- a/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp
+++ b/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp
@@ -1467,14 +1467,26 @@ void CallsiteContextGraph<DerivedCCG, FuncTy, CallTy>::updateStackNodes() {
// of length, and within each length, lexicographically by stack id. The
// latter is so that we can specially handle calls that have identical stack
// id sequences (either due to cloning or artificially because of the MIB
- // context pruning).
- std::stable_sort(Calls.begin(), Calls.end(),
- [](const CallContextInfo &A, const CallContextInfo &B) {
- auto &IdsA = std::get<1>(A);
- auto &IdsB = std::get<1>(B);
- return IdsA.size() > IdsB.size() ||
- (IdsA.size() == IdsB.size() && IdsA < IdsB);
- });
+ // context pruning). Those with the same Ids are then sorted by function to
+ // facilitate efficiently mapping them to the same context node.
+ // Because the functions are pointers, to ensure a stable sort first assign
+ // each function pointer to its first index in the Calls array, and then use
+ // that to sort by.
+ DenseMap<const FuncTy *, unsigned> FuncToIndex;
+ for (const auto &[Idx, CallCtxInfo] : enumerate(Calls))
+ FuncToIndex.insert({std::get<2>(CallCtxInfo), Idx});
+ std::stable_sort(
+ Calls.begin(), Calls.end(),
+ [&FuncToIndex](const CallContextInfo &A, const CallContextInfo &B) {
+ auto &IdsA = std::get<1>(A);
+ auto &IdsB = std::get<1>(B);
+ auto *FuncA = std::get<2>(A);
+ auto *FuncB = std::get<2>(B);
+ return IdsA.size() > IdsB.size() ||
+ (IdsA.size() == IdsB.size() &&
+ (IdsA < IdsB ||
+ (IdsA == IdsB && FuncToIndex[FuncA] < FuncToIndex[FuncB])));
+ });
// Find the node for the last stack id, which should be the same
// across all calls recorded for this id, and is the id for this
@@ -1492,18 +1504,35 @@ void CallsiteContextGraph<DerivedCCG, FuncTy, CallTy>::updateStackNodes() {
DenseSet<uint32_t> LastNodeContextIds = LastNode->getContextIds();
assert(!LastNodeContextIds.empty());
- // Map from function to the first call from the below list (with matching
- // stack ids) found in that function. Note that calls from different
- // functions can have the same stack ids because this is the list of stack
- // ids that had (possibly pruned) nodes after building the graph from the
- // allocation MIBs.
- DenseMap<const FuncTy *, CallInfo> FuncToCallMap;
+#ifndef NDEBUG
+ // Save the set of functions seen for a particular set of the same stack
+ // ids. This is used to ensure that they have been correctly sorted to be
+ // adjacent in the Calls list, since we rely on that to efficiently place
+ // all such matching calls onto the same context node.
+ DenseSet<const FuncTy *> MatchingIdsFuncSet;
+#endif
for (unsigned I = 0; I < Calls.size(); I++) {
auto &[Call, Ids, Func, SavedContextIds] = Calls[I];
assert(SavedContextIds.empty());
assert(LastId == Ids.back());
+#ifndef NDEBUG
+ // If this call has a different set of ids than the last one, clear the
+ // set used to ensure they are sorted properly.
+ if (I > 0 && Ids != std::get<1>(Calls[I - 1]))
+ MatchingIdsFuncSet.clear();
+ else
+ // If the prior call had the same stack ids this set would not be empty.
+ // Check if we already have a call that "matches" because it is located
+ // in the same function. If the Calls list was sorted properly we should
+ // not encounter this situation as all such entries should be adjacent
+ // and processed in bulk further below.
+ assert(!MatchingIdsFuncSet.contains(Func));
+
+ MatchingIdsFuncSet.insert(Func);
+#endif
+
// First compute the context ids for this stack id sequence (the
// intersection of the context ids of the corresponding nodes).
// Start with the remaining saved ids for the last node.
@@ -1572,22 +1601,26 @@ void CallsiteContextGraph<DerivedCCG, FuncTy, CallTy>::updateStackNodes() {
continue;
}
- // If the prior call had the same stack ids this map would not be empty.
- // Check if we already have a call that "matches" because it is located
- // in the same function.
- if (FuncToCallMap.contains(Func)) {
- // Record the matching call found for this call, and skip it. We
- // will subsequently combine it into the same node.
- CallToMatchingCall[Call] = FuncToCallMap[Func];
- continue;
- }
-
// Check if the next set of stack ids is the same (since the Calls vector
// of tuples is sorted by the stack ids we can just look at the next one).
+ // If so, save them in the CallToMatchingCall map so that they get
+ // assigned to the same context node, and skip them.
bool DuplicateContextIds = false;
- if (I + 1 < Calls.size()) {
- auto NextIds = std::get<1>(Calls[I + 1]);
- DuplicateContextIds = Ids == NextIds;
+ for (unsigned J = I + 1; J < Calls.size(); J++) {
+ auto &NextIds = std::get<1>(Calls[J]);
+ if (NextIds != Ids)
+ break;
+ auto *NextFunc = std::get<2>(Calls[J]);
+ if (NextFunc != Func) {
+ // We have another Call with the same ids but that cannot share this
+ // node, must duplicate ids for it.
+ DuplicateContextIds = true;
+ break;
+ }
+ auto &NextCall = std::get<0>(Calls[J]);
+ CallToMatchingCall[NextCall] = Call;
+ // Update I so that it gets incremented correctly to skip this call.
+ I = J;
}
// If we don't have duplicate context ids, then we can assign all the
@@ -1611,14 +1644,7 @@ void CallsiteContextGraph<DerivedCCG, FuncTy, CallTy>::updateStackNodes() {
set_subtract(LastNodeContextIds, StackSequenceContextIds);
if (LastNodeContextIds.empty())
break;
- // No longer possibly in a sequence of calls with duplicate stack ids,
- // clear the map.
- FuncToCallMap.clear();
- } else
- // Record the call with its function, so we can locate it the next time
- // we find a call from this function when processing the calls with the
- // same stack ids.
- FuncToCallMap[Func] = Call;
+ }
}
}
|
snehasish
approved these changes
Sep 10, 2024
I agree, I'll prep an nfc patch to do that. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/134/builds/5014 Here is the relevant piece of the build log for the reference |
teresajohnson
added a commit
to teresajohnson/llvm-project
that referenced
this pull request
Sep 10, 2024
As suggested in llvm#107918, improve readability by converting this tuple to a struct.
teresajohnson
added a commit
that referenced
this pull request
Sep 10, 2024
As suggested in #107918, improve readability by converting this tuple to a struct.
teresajohnson
added a commit
to teresajohnson/llvm-project
that referenced
this pull request
Sep 13, 2024
…tion (llvm#107918)" This reverts commit 524a028, but manually so that follow on PR108086 / ae5f1a7 is retained (NFC patch to convert tuple to a struct).
teresajohnson
added a commit
that referenced
this pull request
Sep 13, 2024
teresajohnson
added a commit
to teresajohnson/llvm-project
that referenced
this pull request
Sep 25, 2024
…ation (llvm#107918)" (llvm#108652) This reverts commit 12d4769, reapplying 524a028 but with fixes for failures seen in broader testing.
teresajohnson
added a commit
that referenced
this pull request
Sep 26, 2024
Sterling-Augustine
pushed a commit
to Sterling-Augustine/llvm-project
that referenced
this pull request
Sep 27, 2024
…ation (llvm#107918)" (llvm#110036) This reverts commit 12d4769, reapplying 524a028 but with fixes for failures seen in broader testing.
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.
Sort the list of calls such that those with the same stack ids are also
sorted by function. This allows processing of all matching calls (that
can share a context node) in bulk as they are all adjacent.
This has 2 benefits:
the context ids with those along the graph edges for the stack ids,
for calls that we know can share a node.
need to duplicate context ids. Specifically, we were previously
still duplicating context ids whenever we saw another call with the
same stack ids, but that isn't necessary if they will share a context
node. With this change we now only duplicate context ids if we see
some that not only have the same ids but also are in different
functions.
This change reduced the amount of context id duplication and provided
reductions in both both peak memory (
8%) and time (%5) for a largetarget.