This is an archive of the discontinued LLVM Phabricator instance.

[profile] Decommit memory after counter relocation
ClosedPublic

Authored by phosek on Jun 23 2021, 10:28 PM.

Details

Summary

After we relocate counters, we no longer need to keep the original copy
around so we can return the memory back to the operating system.

Diff Detail

Event Timeline

phosek created this revision.Jun 23 2021, 10:28 PM
phosek requested review of this revision.Jun 23 2021, 10:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2021, 10:28 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
vsk added inline comments.Jun 24 2021, 3:25 PM
compiler-rt/lib/profile/InstrProfilingUtil.c
346

See InstrProfilingPort.h "getpagesize", this looks like it can replace that.

compiler-rt/lib/profile/InstrProfilingUtil.h
80

Should these helpers assert that boundary is a power of two (not sure what the clearest way would be to check -- maybe assert(popcnt(b) == 1))?

davidxl added inline comments.Jun 25 2021, 10:04 AM
compiler-rt/lib/profile/InstrProfilingFile.c
557

Is this portable across all platforms?

compiler-rt/lib/profile/InstrProfilingPlatformFuchsia.c
199

The snippet is very similar to the one in InstProfilingFile.c -- can they be commoned?

mcgrathr accepted this revision.Jul 5 2021, 12:24 PM

Factoring out the arithmetic from the OS-specific code and landing only the Fuchsia version of the OS-specific implementation LGTM.

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

Unlikely. It may work on some systems other than Linux, but you'd have to check.
Another option is posix_madvise, which is in theory more portable but again you'd have to check which systems actually have it, and its specified semantics are weaker than what you'd like to use here.

Because of the uncertainty of the portable solution for other platforms, I'd recommend that we just land a change affecting only Fuchsia first.

compiler-rt/lib/profile/InstrProfilingPlatformFuchsia.c
199

I agree it would be reasonable to consolidate this arithmetic into a call to an OS-specific function akin to sanitizer_common's ReleaseMemoryPagesToOS. That refactoring still fits well if you start out with only the Fuchsia implementation actually doing anything and leave the other ones as an empty TODO stub until the portability issues are worked out.

compiler-rt/lib/profile/InstrProfilingUtil.c
346

Agreed. There is also __llvm_profile_set_page_size, used only when a particular % magic sequence is found in an option setting (!). All that stuff should be cleaned up one way or another.

compiler-rt/lib/profile/InstrProfilingUtil.h
80

I don't think that's necessary. This kind of alignment rounding is common and always has the power-of-two constraint.

This revision is now accepted and ready to land.Jul 5 2021, 12:24 PM
phosek updated this revision to Diff 357341.Jul 8 2021, 1:34 PM
phosek marked 6 inline comments as done.
phosek added inline comments.
compiler-rt/lib/profile/InstrProfilingFile.c
557

I forgot about Windows and it turned out that implementing MADV_DONTNEED is straightforward so I did that.

phosek added a comment.Jul 9 2021, 1:57 PM

Do you have any other comments? If not I'd like to go ahead and land this change.

davidxl accepted this revision.Jul 9 2021, 2:02 PM

lgtm

This revision was landed with ongoing or failed builds.Jul 15 2021, 10:49 PM
This revision was automatically updated to reflect the committed changes.