This is an archive of the discontinued LLVM Phabricator instance.

[profile] in-process profile merging support Part-3
ClosedPublic

Authored by davidxl on Jun 6 2016, 10:44 PM.

Details

Summary

With this patch, the merging feature will be fully functional.

There are two way to turn on profile merging: command line and environment variable. They both share the same mechanism %[0-9]m specifiler in the name pattern. The numeric modifier in the %m specifier specifies the size of the shared profile file pool size. By default %m is equivalent to %1m which means all processes from the same binary share one single profile file.

Example. Do an instrumented clang build with option -fprofile-instr-generate=/tmp/clang_raw_profile_%m and run clang test or do a clang bootstrap, all instrumented binaries such as clang, opt, llc, llvm-profdata etc will dump profile data into its own profile data file.

Running instrumented binary with LLVM_PROFILE_FILE=..._%m will have the same effect.

Diff Detail

Repository
rL LLVM

Event Timeline

davidxl updated this revision to Diff 59826.Jun 6 2016, 10:44 PM
davidxl retitled this revision from to [profile] in-process profile merging support Part-3.
davidxl updated this object.
davidxl added reviewers: vsk, silvas.
davidxl added a subscriber: llvm-commits.
vsk edited edge metadata.Jun 7 2016, 12:25 AM

Awesome! Some comments/questions inline --

lib/profile/InstrProfilingFile.c
99 ↗(On Diff #59826)

Possible dropped error here

102 ↗(On Diff #59826)

Ditto

104 ↗(On Diff #59826)

Could we skip these two checks by moving the call to __llvm_profile_check_compatibility here?

110 ↗(On Diff #59826)

0 -> NULL

112 ↗(On Diff #59826)

MAP_FAILED instead of -1?

149 ↗(On Diff #59826)

I think any successful call to doProfileMerging ought to be followed by COMPILER_RT_FTRUNCATE. Sink it into the function?

155 ↗(On Diff #59826)

Ditto, fseek error

245 ↗(On Diff #59826)

Should this check be: if (MergingEnabled && (FilenamePat[I] == 'm' || FilenamePat[I + 1] == 'm'))? Why check whether the character after the 'm' is nil?

280 ↗(On Diff #59826)

Could you add a comment explaining that '24' comes from lprofGetLoadModuleSignature? In fact, it might be better to hide this detail in a helper function in InstrProfilingMerge.c.

326 ↗(On Diff #59826)

Can this predicate be shared with parseFilenamePattern? I'd prefer not to have it duplicated.

test/profile/instrprof-basic.c
18 ↗(On Diff #59826)

Is the lprofGetLoadModuleSignature code path covered?

davidxl marked 8 inline comments as done.Jun 7 2016, 9:45 AM
davidxl added inline comments.
lib/profile/InstrProfilingFile.c
104 ↗(On Diff #59826)

yes

149 ↗(On Diff #59826)

doProfileMerge just merges profile into memory, and the wrapper OpenFileForMerge does the rest to prepare for writing (including truncating). Another reason it is here is that doProfileMerge has multiple 'return 0' so extracting truncation code out is cleaner.

245 ↗(On Diff #59826)

The second error condition is that %m specifier is not at the end of the pat. This restriction is for convenience. I don't see a reason to support %[0-9]m in arbitrary locations.

test/profile/instrprof-basic.c
18 ↗(On Diff #59826)

The plan is to have basic test coverage for now. More tests will be added as follow up as things stablizes.

davidxl updated this revision to Diff 59905.Jun 7 2016, 10:17 AM
davidxl edited edge metadata.
davidxl marked an inline comment as done.

Addressed vsk's comments.

vsk added inline comments.Jun 7 2016, 11:47 AM
lib/profile/InstrProfilingFile.c
149 ↗(On Diff #59905)

Ok

238 ↗(On Diff #59905)

I think "binary-%m.profraw" could be a common pattern; moreover I don't see a reason to not support this. Would removing this restriction require adding more code to getCurFilenameLength()/getCurFilename()?

test/profile/instrprof-basic.c
18 ↗(On Diff #59905)

Ok

davidxl updated this revision to Diff 59951.Jun 7 2016, 2:25 PM

Relax the constraint that %m can only be at the end of the pattern. It can now appear anywhere.

vsk accepted this revision.Jun 7 2016, 2:56 PM
vsk edited edge metadata.

LGTM with one nit.

lib/profile/InstrProfilingFile.c
34 ↗(On Diff #59951)

I don't think this is used anywhere, and it might be a bit cumbersome. Delete?

This revision is now accepted and ready to land.Jun 7 2016, 2:56 PM
silvas edited edge metadata.Jun 7 2016, 10:07 PM

Looks like Vedant covered most of the things I wanted to mention. LGTM with a couple nits.

lib/profile/InstrProfilingFile.c
226 ↗(On Diff #59951)

Naively, this seems like it may read off the end. A comment explaining why not would probably be helpful.

296 ↗(On Diff #59951)

typo: ty should be by

lib/profile/InstrProfilingInternal.h
160 ↗(On Diff #59951)

I think you meant "executable" instead of "executation".

lib/profile/InstrProfilingMerge.c
24 ↗(On Diff #59951)

I think it is more accurate to say "a module signature" instead of "the module signature". Ideally we would e.g. hash all the names or something else a bit more robust (but I think that just the sizes will be fine for now).

davidxl marked 4 inline comments as done.Jun 8 2016, 9:35 AM
davidxl added inline comments.
lib/profile/InstrProfilingInternal.h
160 ↗(On Diff #59951)

Fixed.

This revision was automatically updated to reflect the committed changes.