This is an archive of the discontinued LLVM Phabricator instance.

[Profiling] Write out sparse indexed profiles
ClosedPublic

Authored by vsk on Jan 29 2016, 8:41 AM.

Details

Summary

Instead of writing out InstrProfRecords which contain only 0 counts, skip them.

The Xcode team asked for this so that the (often sparse) profiles gathered from UI tests take up less space.

Diff Detail

Repository
rL LLVM

Event Timeline

vsk updated this revision to Diff 46388.Jan 29 2016, 8:41 AM
vsk retitled this revision from to [Profiling] Write out sparse indexed profiles.
vsk updated this object.
vsk added reviewers: davidxl, slingn.
vsk added a subscriber: llvm-commits.
davidxl edited edge metadata.Jan 29 2016, 9:14 AM

Any reason why we can not just always emit sparse output ?

unittests/ProfileData/CoverageMappingTest.cpp
291 ↗(On Diff #46388)

what is this change about?

slingn edited edge metadata.EditedJan 29 2016, 9:19 AM

Making sparse the default would change the output of llvm-profdata show since zero count functions would no longer be listed. Is that a concern? In other words, could existing users of llvm-profdata be relying on it in any way?

include/llvm/ProfileData/InstrProfWriter.h
64 ↗(On Diff #46388)

I'm also not sure about 'Useful' as the name - since zeros are useful - just not necessary to write out.

How about shouldEncodeData()?

lib/ProfileData/InstrProfWriter.cpp
153 ↗(On Diff #46388)

Since this takes a boolean I would suggest naming it setSparseOutput(bool Sparse)

tools/llvm-profdata/llvm-profdata.cpp
231 ↗(On Diff #46388)

I would invert this to say 'only meaningful for instrumentation profiles'.

If there is to be a new option for llvm-profdata then the *.rst documentation file needs to be updated too:
llvm/docs/CommandGuide/llvm-profdata.rst

vsk marked 3 inline comments as done.Jan 29 2016, 10:00 AM

I think we should keep the non-sparse mode. Users of llvm-profdata might rely on having records for functions which don't execute (though I imagine this is a small group). In addition, one of our tests assumes this (weight-instr.test). Having -sparse turned on by default seems fair to me.

unittests/ProfileData/CoverageMappingTest.cpp
291 ↗(On Diff #46388)

IIUC, changing the counter from 10 to 0 doesn't alter the spirit of this test.

The change lets us check that we behave correctly when we don't write Record out.

vsk updated this revision to Diff 46393.Jan 29 2016, 10:01 AM
vsk edited edge metadata.
  • Addressed sling's comments.
davidxl added inline comments.Jan 29 2016, 10:22 AM
unittests/ProfileData/CoverageMappingTest.cpp
291 ↗(On Diff #46393)

So why just this one change. For instance, the previous test can also be changed with {0, 0} counts to cover both the sparse and non sparse paths.

I have one major concern with this patch:

When the records for those zero count functions are skipped, the profile-use compilation may get confused -- those functions are now treated as 'unknown functions' which the compiler can not do anything about them. One of the main benefit of keeping zero count functions is that the compiler can do very aggressive size optimizations on them.

If we go with this route, we need a way to store place holder records for the cold functions.

If we do care about size of indexed profile data, name compression will be very efficient way. My pending patch does not touch Indexed profile format, but it can be easily extended to Indexed format (and also keeping the ability to read the old format files).

lib/ProfileData/InstrProfWriter.cpp
197 ↗(On Diff #46393)

Is this check needed?

vsk added a comment.Jan 29 2016, 1:19 PM

(Sorry - typo earlier, slingn's*.)

unittests/ProfileData/CoverageMappingTest.cpp
291 ↗(On Diff #46393)

Changing this one test lets us check that we get coverage information for functions which don't execute in sparse mode. I'll update the test you mentioned so it has records with zero and non-zero count.

vsk updated this revision to Diff 46419.Jan 29 2016, 1:20 PM
  • Extend a unittest in CoverageMappingTest.
vsk added a comment.Jan 29 2016, 1:24 PM

Good point. Would making -sparse opt-in help with this? That way if someone really cares about trimming down the profdata file (e.g if optimization is not a concern), they have one more option. Having this in addition to your name compression patch can't hurt.

vsk updated this revision to Diff 46421.Jan 29 2016, 1:31 PM
  • Turn off -sparse by default to address @davidxl's concern. This has the added benefit of letting us leave weight-instr.test unchanged.

that sounds good to me.

David

vsk updated this revision to Diff 46422.Jan 29 2016, 1:34 PM
  • Remove dead check.
davidxl added inline comments.Jan 29 2016, 1:41 PM
lib/ProfileData/InstrProfWriter.cpp
298 ↗(On Diff #46422)

This guard is not right -- the function may be referenced by an indirect call target from another emitted function. In fact since text format is mainly used for debugging, I don't see much point honoring sparse flag here.

Discard the first part of the comment -- that scenario can not exist.

David

davidxl accepted this revision.Jan 29 2016, 2:34 PM
davidxl edited edge metadata.

The text dump issue is minor. LGTM

This revision is now accepted and ready to land.Jan 29 2016, 2:34 PM
This revision was automatically updated to reflect the committed changes.