Page MenuHomePhabricator

[profile] Support counter relocation at runtime
ClosedPublic

Authored by phosek on Nov 1 2019, 4:11 PM.

Details

Summary

This is an alternative to the continous mode that was implemented in
D68351. This mode relies on padding and the ability to mmap a file over
the existing mapping which is generally only available on POSIX systems
and isn't suitable for other platforms.

This change instead introduces the ability to relocate counters at
runtime using a level of indirection. On every counter access, we add a
bias to the counter address. This bias is stored in a symbol that's
provided by the profile runtime and is initially set to zero, meaning no
relocation. The runtime can mmap the profile into memory at abitrary
location, and set bias to the offset between the original and the new
counter location, at which point every subsequent counter access will be
to the new location, which allows updating profile directly akin to the
continous mode.

The advantage of this implementation is that doesn't require any special
OS support. The disadvantage is the extra overhead due to additional
instructions required for each counter access (overhead both in terms of
binary size and performance) plus duplication of counters (i.e. one copy
in the binary itself and another copy that's mmapped).

Diff Detail

Event Timeline

phosek created this revision.Nov 1 2019, 4:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2019, 4:11 PM
phosek added a comment.Nov 1 2019, 4:17 PM

This is an alternative to D69700. I'm still working on tests but wanted to share this to get some early feedback. One open question is how to control when to enable the continuous profile update. Currently it's always enabled when -mllvm -runtime-counter-relocation=true was using during compilation, but the runtime behavior is technically orthogonal (i.e. you can decide to compile with relocatable counters but don't use the online profile sync at runtime) so maybe there should be a separate mechanism. The continuous mode that landed in D68351 uses the %c filename pattern, but don't support filename patterns nor environment variables on Fuchsia; we could use -mllvm -runtime-counter-relocation=true + %c on other platforms, whereas on Fuchsia this mode would be always used by default.

vsk added a comment.Nov 4 2019, 2:24 PM

It's possible to use __llvm_profile_set_filename to enable continuous mode. Is that an option for Fuchsia? If not, it seems reasonable to make continuous mode the default there.

vsk added inline comments.Nov 4 2019, 2:25 PM
compiler-rt/lib/profile/InstrProfilingFile.c
98

I believe this needs to be visible to InstrProfilingBuffer.c so that __llvm_profile_get_padding_sizes_for_counters can set the padding amounts to 0.

If the runtime/size overhead is acceptable, I prefer this simpler version of implementation. We can always document the improvement possibility in the source and enhance it later if needed.

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

Can you split the refactoring change into a separate patch to minimize diff here?

vsk added inline comments.Nov 12 2019, 10:27 AM
compiler-rt/lib/profile/InstrProfilingFile.c
252

I believe this code is borrowed from D69586. I'll split the refactoring bits out from that review and start a fresh one: D70135

Herald added a project: Restricted Project. · View Herald TranscriptNov 15 2019, 3:02 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
vsk added a comment.Nov 19 2019, 12:48 PM

I think this looks good overall. The only high-level item that seems to be missing is Fuchsia testing -- are there any bots for this?

I've left inline comments about fixing up some tests. Please also leave a comment about Fuchsia/-runtime-counter-relocation in clang/docs/SourceBasedCodeCoverage.rst.

compiler-rt/lib/profile/InstrProfiling.h
315

Could you add __llvm_profile_counter_bias to clang's Darwin::addProfileRTLibs, to appease tapi? This will prevent breakage in the instrprof-darwin-exports.c test.

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

I believe this adds a dependency from InstrProfilingBuffer.c onto InstrProfilingFile.c onto, which I think breaks the instrprof-without-libc test. I think RuntimeCounterRelocation needs to live in InstrProfilingBuffer.c.

phosek updated this revision to Diff 230727.Nov 22 2019, 2:53 PM
phosek marked 2 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptNov 22 2019, 2:53 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
phosek updated this revision to Diff 230733.Nov 22 2019, 3:11 PM
In D69740#1752348, @vsk wrote:

I think this looks good overall. The only high-level item that seems to be missing is Fuchsia testing -- are there any bots for this?

Not yet unfortunately, it's something I'm slowly working on but it'll take more time. I've been only testing this manually for now.

I've left inline comments about fixing up some tests. Please also leave a comment about Fuchsia/-runtime-counter-relocation in clang/docs/SourceBasedCodeCoverage.rst.

Done.

vsk accepted this revision.Dec 2 2019, 8:50 AM

Lgtm with a small cleanup.

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
657

Could you lazily insert the bias load inside of lowerIncrement? That removes the need to delete the load when no increment is found.

This revision is now accepted and ready to land.Dec 2 2019, 8:50 AM
phosek updated this revision to Diff 238689.Jan 16 2020, 7:58 PM
phosek marked an inline comment as done.

Addressed the comment and also added a test for Linux.

This revision was automatically updated to reflect the committed changes.