This is an archive of the discontinued LLVM Phabricator instance.

[SampleFDO] Compute and report profile staleness metrics
ClosedPublic

Authored by wlei on Oct 24 2022, 10:51 AM.

Details

Summary

When a profile is stale and profile mismatch could happen, the mismatched samples are discarded, so we'd like to compute the mismatch metrics to quantify how stale the profile is, which will suggest user to refresh the profile if the number is high.

Two sets of metrics are introduced here:

  • (Num_of_mismatched_funchash/Total_profiled_funchash), (Samples_of_mismached_func_hash / Samples_of_profiled_function) : Here it leverages the FunctionSamples's checksums attribute which is a feature of pseudo probe. When the source code CFG changes, the function checksums will be different, later sample loader will discard the whole functions' samples, this metrics can show the percentage of samples are discarded due to this.
  • (Num_of_mismatched_callsite/Total_profiled_callsite), (Samples_of_mismached_callsite / Samples_of_profiled_callsite) : This shows how many mismatching for the callsite location as callsite location mismatch will affect the inlining which is highly correlated with the performance. It goes through all the callsite location in the IR and profile, use the call target name to match, report the num of samples in the profile that doesn't match a IR callsite.

This is implemented in a new class(SampleProfileMatcher) and under a switch("--report-profile-staleness"), we plan to extend it with a fuzzy profile matching feature in the future.

Diff Detail

Event Timeline

wlei created this revision.Oct 24 2022, 10:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 24 2022, 10:51 AM
wlei requested review of this revision.Oct 24 2022, 10:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 24 2022, 10:51 AM
wlei retitled this revision from [AutoFDO] Compute profile mismatch metrics to [SampleFDO] Compute profile mismatch metrics.Oct 24 2022, 12:00 PM
wlei edited the summary of this revision. (Show Details)
wlei added reviewers: hoy, wenlei, xur, davidxl.
davidxl added inline comments.Oct 24 2022, 12:22 PM
llvm/lib/Transforms/IPO/SampleProfile.cpp
132

The stats seems like a useful thing independent of fuzzy matching. Should it be controlled with a different option?

2072

skip non calls earlier in the loop to reduce nesting level.

2089

what does this case cover? inlined indirect call?

2101

use symbolic constant?

wenlei added inline comments.Oct 24 2022, 12:55 PM
llvm/lib/Transforms/IPO/SampleProfile.cpp
132

agreed. something like -report-profile-staleness?

430

How about having another metric to represent how many functions has mismatched profile? That should tell the breath of the mismatch while your current metric tells the severity of mismatch.

2051

nit: white space line in between.

2069–2072

Intrinsic is narrower than Call, the checks seem out of order.

2083–2084

For readability, restructure it this way?

if (CalleeName.empty())  // indirect call in IR
   ...
else                                  // direct call in IR 
   ...
2086

nit: CTM->find(CalleeName) != CTM->end() -> CTM->count(CalleeName)

2090

nit: CallsiteFS->find(CalleeName) != CallsiteFS->end())) -> CallsiteFS->count(CalleeName)

2134–2137

Is this always going to be 0 for non-probe case? Should we omit this message when probe isn't used?

davidxl added inline comments.Oct 24 2022, 1:13 PM
llvm/lib/Transforms/IPO/SampleProfile.cpp
132

sounds good.

wlei marked 3 inline comments as done.Oct 24 2022, 2:26 PM
wlei added inline comments.
llvm/lib/Transforms/IPO/SampleProfile.cpp
132

Good point, replaced the sample-profile-fuzzy-match, will add it back when fuzzy match is ready.

430

Sounds good.(We already have the same stats(https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/IPO/SampleProfile.cpp#L1752) but it's only available with the -stats).

2051

fixed, thanks!

2069–2072

fixed, thanks!

2089

Yes, this is to avoid false positives, otherwise all the indirect call function samples will be reported as mismatching, updated the comments.

2101

fixed, thanks!

2134–2137

Sounds good, made it under the probe condition.

wlei updated this revision to Diff 470290.Oct 24 2022, 2:26 PM
wlei edited the summary of this revision. (Show Details)

addressing reviewers' feedback

hoy added inline comments.Oct 24 2022, 2:51 PM
llvm/lib/Transforms/IPO/SampleProfile.cpp
2108

Should invalid line offset also be considered mismatched samples? The samples will discarded any way.

BTW, sort of having an impression that the invalid line offset checking is used anywhere else. Can we unify the usage if that's the case?

2212

nit: rename matchProfiles something like detectProfileMismatch directly? The name of the switch sounds like only mismatch detection should be done here.

wlei added inline comments.Oct 24 2022, 3:22 PM
llvm/lib/Transforms/IPO/SampleProfile.cpp
2108

From my observation, the invalid(negative) offset will never be matched to any offset in the IR, so we actually need to remove it from the profile rather than report as mismatching. Recalling we tried to remove this offset in llvm-profgen, however, it caused a regression(likely because it affect the sample hot cutoff), so we chose to keep it. As for the mismatch, I'm thinking to report the samples that's caused by the real stale profile issue, this might mislead the user.

BTW, sort of having an impression that the invalid line offset checking is used anywhere else. Can we unify the usage if that's the case?

yeah, that's from llvm-profgen, but that diff is reverted.

2212

Fixed, thanks!

wlei updated this revision to Diff 470303.Oct 24 2022, 3:22 PM

rename matchProfiles --> detectProfileMismatch

wenlei added inline comments.Oct 24 2022, 3:56 PM
llvm/lib/Transforms/IPO/SampleProfile.cpp
2144

When the flag is on, we should always emit the messages, regardless of debug build or not. Same for the one below.

wlei updated this revision to Diff 470323.Oct 24 2022, 4:39 PM

Updating D136627: [SampleFDO] Compute profile mismatch metrics

llvm/lib/Transforms/IPO/SampleProfile.cpp
2144

Sounds good! Changed to use outs

hoy added inline comments.Oct 24 2022, 5:13 PM
llvm/lib/Transforms/IPO/SampleProfile.cpp
2069–2072

nit: maybe more explicit:

if (!isa<CallInst>(I) && !isa<InvokeInst>(I))
     continue;
2108

Sounds good to exclude invalid line offsets.

yeah, that's from llvm-profgen, but that diff is reverted.

I see. Factor out the checking logic here to be a general function, maybe a static function of LineLocation?

wenlei added inline comments.Oct 24 2022, 8:33 PM
llvm/lib/Transforms/IPO/SampleProfile.cpp
430

Sorry I wasn't explicit enough, but I was thinking about doing the same for both call site and function. I.e. have both samples mismatch and number of function/callsite mismatch.

2108

IIRC, these represents sample that can't be attributed to a particular line, but excluding them in profile generation changes total sample of a function, which led to regression.

I think excluding them in mismatch count makes sense because there's not much user can do about these, i.e. a profile refresh won't make them go away.

2144

I don't know if there's well established convention, but I think most of similar dump go to stderr (i.e. all IR dump goes to stderr).

wenlei added inline comments.Oct 24 2022, 8:46 PM
llvm/include/llvm/ProfileData/SampleProf.h
308–309

It doesn't matter on our platform, but just pointing out that this code isn't portable on host target with size_t -> uint32_t, in which case, the hash becomes just Loc.Discriminator.

wlei added inline comments.Oct 25 2022, 10:34 AM
llvm/include/llvm/ProfileData/SampleProf.h
308–309

This is unintentional, thanks for pointing this out. changed to uint64_t

llvm/lib/Transforms/IPO/SampleProfile.cpp
430

I see, added callsite num.

2069–2072

Tried this, but I found this !isa<InvokeInst>(I) doesn't work for the test in this diff. I found other places in the sampleloader are all using the isa<IntrinsicInst>(&I) check, maybe just be consistent with other place?

2108

I see. Factor out the checking logic here to be a general function, maybe a static function of LineLocation?

Sorry I meant the usage in llvm-profgen is removed and now here is the only place to use this check, I'm not very sure if this is a standard way to check if offset is valid(to make it in LineLocation), so how about just leave it here(refactor when other places used it in the future)?

2144

Yeah, you're right, dumping into stdout will mess up the IR and make it hard to recompile the IR. Changed to stderr

wlei updated this revision to Diff 470557.Oct 25 2022, 10:36 AM

Updating D136627: [SampleFDO] Compute profile mismatch metrics

hoy added inline comments.Oct 25 2022, 10:58 AM
llvm/lib/Transforms/IPO/SampleProfile.cpp
2108

Sounds good.

2154

Still seeing dbgs() here. Should it be stderr?

wlei added inline comments.Oct 25 2022, 12:08 PM
llvm/lib/Transforms/IPO/SampleProfile.cpp
2154

I see, I thought dbgs() is the same thing to errs() for non-debug mode.. changed to errs(), thanks!

wlei updated this revision to Diff 470588.Oct 25 2022, 12:09 PM

Updating D136627: [SampleFDO] Compute profile mismatch metrics

wenlei accepted this revision.Oct 25 2022, 12:11 PM

lgtm, thanks!

This revision is now accepted and ready to land.Oct 25 2022, 12:11 PM
hoy accepted this revision.Oct 25 2022, 1:13 PM
davidxl accepted this revision.Oct 25 2022, 1:56 PM

lgtm

wlei retitled this revision from [SampleFDO] Compute profile mismatch metrics to [SampleFDO] Compute and report profile staleness metrics.Oct 26 2022, 8:54 PM
wlei edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.