-
Notifications
You must be signed in to change notification settings - Fork 13k
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
[CSSPGO] Fix redundant reading of profile metadata #129609
Conversation
@llvm/pr-subscribers-pgo Author: Lei Wang (wlei-llvm) ChangesFix a build speed regression due to repeated reading of profile metadata. Before the function Full diff: https://github.com/llvm/llvm-project/pull/129609.diff 2 Files Affected:
diff --git a/llvm/include/llvm/ProfileData/SampleProfReader.h b/llvm/include/llvm/ProfileData/SampleProfReader.h
index 5301f23def3f3..76c7cecded629 100644
--- a/llvm/include/llvm/ProfileData/SampleProfReader.h
+++ b/llvm/include/llvm/ProfileData/SampleProfReader.h
@@ -777,7 +777,7 @@ class SampleProfileReaderExtBinaryBase : public SampleProfileReaderBinary {
std::error_code readSecHdrTable();
std::error_code readFuncMetadata(bool ProfileHasAttribute,
- SampleProfileMap &Profiles);
+ DenseSet<FunctionSamples *> &Profiles);
std::error_code readFuncMetadata(bool ProfileHasAttribute);
std::error_code readFuncMetadata(bool ProfileHasAttribute,
FunctionSamples *FProfile);
diff --git a/llvm/lib/ProfileData/SampleProfReader.cpp b/llvm/lib/ProfileData/SampleProfReader.cpp
index 98c7844378527..d8b92b920b0aa 100644
--- a/llvm/lib/ProfileData/SampleProfReader.cpp
+++ b/llvm/lib/ProfileData/SampleProfReader.cpp
@@ -831,8 +831,15 @@ SampleProfileReaderExtBinaryBase::read(const DenseSet<StringRef> &FuncsToUse,
if (std::error_code EC = readFuncProfiles(FuncsToUse, Profiles))
return EC;
End = Data;
+ DenseSet<FunctionSamples *> ProfilesToReadMetadata;
+ for (auto FName : FuncsToUse) {
+ auto I = Profiles.find(FName);
+ if (I != Profiles.end())
+ ProfilesToReadMetadata.insert(&I->second);
+ }
- if (std::error_code EC = readFuncMetadata(ProfileHasAttribute, Profiles))
+ if (std::error_code EC =
+ readFuncMetadata(ProfileHasAttribute, ProfilesToReadMetadata))
return EC;
return sampleprof_error::success;
}
@@ -1300,14 +1307,12 @@ SampleProfileReaderExtBinaryBase::readFuncMetadata(bool ProfileHasAttribute,
return sampleprof_error::success;
}
-std::error_code
-SampleProfileReaderExtBinaryBase::readFuncMetadata(bool ProfileHasAttribute,
- SampleProfileMap &Profiles) {
+std::error_code SampleProfileReaderExtBinaryBase::readFuncMetadata(
+ bool ProfileHasAttribute, DenseSet<FunctionSamples *> &Profiles) {
if (FuncMetadataIndex.empty())
return sampleprof_error::success;
- for (auto &I : Profiles) {
- FunctionSamples *FProfile = &I.second;
+ for (auto *FProfile : Profiles) {
auto R = FuncMetadataIndex.find(FProfile->getContext().getHashCode());
if (R == FuncMetadataIndex.end())
continue;
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix. LGTM.
@wlei-llvm when was the regression introduced? |
The regression was triggered after we enabled the call-graph matching(#125938), it was initially introduced in #104654 when we refactored to support on-demand reading profiles(call-graph matching is the only usage for this feature). |
Fix a build speed regression due to repeated reading of profile metadata. Before the function
readFuncMetadata(ProfileHasAttribute, Profiles)
reads the metadata for all the functions(Profiles
), however, it's actually used for on-demand loading, it can be called for multiple times, which leads to redundant reading that causes the build speed regression. Now fix it to read the metadata only for the new loaded functions(functions in theFuncsToUse
).