This is an archive of the discontinued LLVM Phabricator instance.

[llvm-profgen][CSSPGO] Support count based aggregated type of hybrid perf script
ClosedPublic

Authored by wlei on Jul 30 2021, 12:28 PM.

Details

Summary

This change tried to integrate a new count based aggregated type of perf script. The only difference of the format is that an aggregated count is added at the head of the original sample which means the same samples are repeated to the given count times. This is used to reduce the perf script size.
e.g.

2
	          4005dc
	          400634
	          400684
	    7f68c5788793
 0x4005c8/0x4005dc/P/-/-/0  ....

Implemented by a dedicated PerfReader AggregatedHybridPerfReader.

Diff Detail

Event Timeline

wlei created this revision.Jul 30 2021, 12:28 PM
wlei requested review of this revision.Jul 30 2021, 12:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2021, 12:28 PM
wlei retitled this revision from [llvm-profgen] Support count based aggregated type of perf script to [llvm-profgen][CSSPGO] Support count based aggregated type of hybrid perf script.Jul 30 2021, 12:40 PM
wlei edited the summary of this revision. (Show Details)
wlei added reviewers: hoy, wenlei, wmi.
wenlei added inline comments.Jul 30 2021, 3:47 PM
llvm/tools/llvm-profgen/PerfReader.h
67

How about we make the aggregation form an "extension" applicable to any perf script type instead of a new type? This is because we could have a count for LBR perf script too (when we use llvm-profgen for line-based AutoFDO too in the future). If we make aggregation a separate type, we would potentially need to double the number of types..

hoy added inline comments.Jul 30 2021, 4:47 PM
llvm/test/tools/llvm-profgen/Inputs/noinline-cs-noprobe.aggperfscript
7

Can you remove the /P/-/-/0 part and see if the existing parser can handle both formats? That part is unnecessary.

llvm/tools/llvm-profgen/PerfReader.h
67

+1, this is a good idea.

wlei updated this revision to Diff 363553.Aug 2 2021, 1:00 PM
wlei edited the summary of this revision. (Show Details)

Addressing feedbacks, change to not use a seperated class for the aggregated script

wenlei added a comment.Aug 2 2021, 1:31 PM

Along the lines of supporting aggregation as an extension, perhaps we could handle parseAggregatedCount in the base class inside parseSample for anything other than stuff like PERF_RECORD_MMAP2. What do you think?

wlei updated this revision to Diff 363613.Aug 2 2021, 7:00 PM

put parseAggregatedCount into base class

wlei added a comment.Aug 2 2021, 7:00 PM

Along the lines of supporting aggregation as an extension, perhaps we could handle parseAggregatedCount in the base class inside parseSample for anything other than stuff like PERF_RECORD_MMAP2. What do you think?

That's good point, so we can share this with the non-cs profile.

wenlei accepted this revision.Aug 2 2021, 8:16 PM

There seems to have some test failures related to unwinder checks. Otherwise lgtm, thanks.

llvm/tools/llvm-profgen/PerfReader.cpp
681

assert that Count >= 1 here?

This revision is now accepted and ready to land.Aug 2 2021, 8:16 PM
hoy accepted this revision.Aug 3 2021, 9:23 AM

Looks like the test failures are due to missing mmap events. Rebasing the current diff on top of previous patch should fix it.

wlei updated this revision to Diff 363792.Aug 3 2021, 10:05 AM

add assertion

wlei added a comment.Aug 3 2021, 12:04 PM

There seems to have some test failures related to unwinder checks. Otherwise lgtm, thanks.
Looks like the test failures are due to missing mmap events. Rebasing the current diff on top of previous patch should fix it.

Yeah, see the recent build, it passed. Seems because I didn't add the dependance of the two patches and the build system doesn't catch this automatically.