Page MenuHomePhabricator

[profile] Support online merging with continuous sync mode
ClosedPublic

Authored by vsk on Oct 29 2019, 3:05 PM.

Details

Summary

Make it possible to use online profile merging ("%m" mode) with
continuous sync ("%c" mode).

To implement this, the merged profile is locked in the runtime
initialization step and either a) filled out for the first time or b)
checked for compatibility. Then, the profile can simply be mmap()'d with
MAP_SHARED set. With the mmap() in place, counter updates from every
process which uses an image are mapped onto the same set of physical
pages assigned by the filesystem cache. After the mmap() is set up, the
profile is unlocked.

Depends on D70135.

Diff Detail

Event Timeline

vsk created this revision.Oct 29 2019, 3:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 29 2019, 3:05 PM
vsk updated this revision to Diff 227520.Nov 1 2019, 1:23 PM

Delete stale comments about merging+continuous mode being unsupported & rebase.

wenlei added a subscriber: wenlei.Nov 4 2019, 11:21 PM
vsk updated this revision to Diff 228919.Nov 12 2019, 10:27 AM
vsk edited the summary of this revision. (Show Details)

Split out the NFC changes into D70135, and rebase on top of it.

davidxl added inline comments.Nov 15 2019, 3:45 PM
compiler-rt/lib/profile/InstrProfilingFile.c
400

why this change?

515

perhaps:

int ret = getProfileFIleSizeForMerging(...);

if (ret == -1 || profileFileSize == 0) {

// full write ...

} else {

// merge

}

548–549

Can this code be moved up to line 499 ? Is ProfileRequiresFullWrite flag still needed?

563

This can be hoisted closer to the merge handling.

vsk updated this revision to Diff 229664.Nov 15 2019, 4:28 PM
vsk marked 5 inline comments as done.
vsk edited the summary of this revision. (Show Details)

Address review feedback.

compiler-rt/lib/profile/InstrProfilingFile.c
400

When continuous+online-merging mode are enabled together, we must create the profile-dir so that continuous mode can be set up (create a profile to mmap()). Otherwise the profile-dir may never be created.

548–549

Yes, thanks.

563

In the success case, the unlock still needs to occur, so if we hoist the unlock up we'd need a 'backwards' goto. I'm not sure that would be cleaner.

Since the unlock is the common path, why not replacing goto with a helper as well?

lprofUnlockIfNeeded(profileRequiresUnlock, File);
return;

vsk updated this revision to Diff 229677.Nov 15 2019, 6:56 PM

Replace 'goto unlockAndReturn' with calls to a helper.

davidxl accepted this revision.Nov 15 2019, 7:12 PM

lgtm

This revision is now accepted and ready to land.Nov 15 2019, 7:12 PM
This revision was automatically updated to reflect the committed changes.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 18 2019, 12:58 PM
Herald added subscribers: Restricted Project, cfe-commits. · View Herald Transcript
aganea added a subscriber: aganea.Nov 19 2019, 12:26 PM
aganea added inline comments.
compiler-rt/lib/profile/InstrProfilingFile.c
419

These two functions are not used when targeting Windows, do you think it'll be possible to #ifdef them out in a follow up patch? Thanks in advance!

[550/4755] Building C object projects\compiler-rt\lib\profile\CMakeFiles\clang_rt.profile-x86_64.dir\InstrProfilingFile.c.obj
F:\llvm-project\compiler-rt\lib\profile\InstrProfilingFile.c(419,12): warning: unused function 'writeProfileWithFileObject' [-Wunused-function]
static int writeProfileWithFileObject(const char *Filename, FILE *File) {
           ^
F:\llvm-project\compiler-rt\lib\profile\InstrProfilingFile.c(429,13): warning: unused function 'unlockProfile' [-Wunused-function]
static void unlockProfile(int *ProfileRequiresUnlock, FILE *File) {
            ^
2 warnings generated.
vsk marked an inline comment as done.Nov 19 2019, 12:53 PM
vsk added inline comments.
compiler-rt/lib/profile/InstrProfilingFile.c
419