This is an archive of the discontinued LLVM Phabricator instance.

[Support] Add MemoryBuffer::dontNeedIfMmap
ClosedPublic

Authored by MaskRay on Dec 29 2021, 1:09 AM.

Details

Summary

On *NIX systems, this API calls madvise(MADV_DONTNEED) on read-only file mappings.
It should not be used on a writable buffer.
The API is used to implement ld.lld LTO memory saving trick (D116367).

Note: on read-only file mappings, Linux's MADV_DONTNEED semantics match POSIX
POSIX_MADV_DONTNEED and BSD systems' MADV_DONTNEED.

On Windows, VirtualAllocEx MEM_COMMIT/MEM_RESET have similar semantics
but are unfortunately not drop-in replacements. dontNeedIfMmap is currently a no-op.

Diff Detail

Unit TestsFailed

Event Timeline

MaskRay created this revision.Dec 29 2021, 1:09 AM
MaskRay requested review of this revision.Dec 29 2021, 1:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 29 2021, 1:09 AM
MaskRay edited the summary of this revision. (Show Details)Dec 29 2021, 1:16 AM
aganea accepted this revision.Dec 29 2021, 8:55 AM

LGTM.

llvm/lib/Support/Unix/Path.inc
874

Since MADV_DONTNEED will drop all written changes to the memory pages, is it worth adding assert(Mode == mapped_file_region::readonly); here?

llvm/lib/Support/Windows/Path.inc
962

Probably the closest we could do on Windows is:

void mapped_file_region::dontNeedImpl() {
  assert(Mode == mapped_file_region::readonly);
  VirtualFree(Mapping, Size, MEM_DECOMMIT);
}

...but that's "destructive", since VirtualAlloc(..MEM_COMMIT..) would be required to access the pages again. That would require another function mapped_file_region::needImpl() ...

There's also VirtualAlloc(..MEM_RESET..) which is closer to MADV_DONTNEED but that doesn't work with mmap'd files.

This revision is now accepted and ready to land.Dec 29 2021, 8:55 AM
MaskRay updated this revision to Diff 396564.Dec 29 2021, 1:04 PM
MaskRay marked an inline comment as done.

Add an assert

llvm/lib/Support/Unix/Path.inc
874

Good idea! Applied.

llvm/lib/Support/Windows/Path.inc
962

Ah, then what can be done with Windows? Probably not many use Windows ld.lld, but Windows lld-link with lower memory usage may be useful to some groups.

For this patch, I'll not touch the Windows implementation if a direct MADV_DONTNEED counterpart cannot be found.

MaskRay updated this revision to Diff 396586.Dec 29 2021, 5:46 PM
MaskRay edited the summary of this revision. (Show Details)

Clarify that dontNeedIfMmap should not be called on a writable buffer (Linux MADV_DONTNEED has unusual destructive semantics)

This revision was landed with ongoing or failed builds.Dec 30 2021, 10:42 AM
This revision was automatically updated to reflect the committed changes.
MaskRay edited the summary of this revision. (Show Details)Dec 30 2021, 11:33 AM
zibi added a subscriber: zibi.Dec 31 2021, 6:02 AM
zibi added inline comments.
llvm/lib/Support/Unix/Path.inc
876

Unfortunately this change broke AIX build since MADV_DONTNEED is not available. Is there any other OS without MADV_DONTNEED? Can this be reverted until we find alternative?

Unfortunately this change broke AIX build since MADV_DONTNEED is not available. Is there any other OS without MADV_DONTNEED? Can this be reverted until we find alternative?

Maybe we can swap to posix_madvise if the semantics match what we need here? That should be available on AIX.

aganea added a subscriber: simon_tatham.EditedDec 31 2021, 10:15 AM

@zibi @daltenty Is madvise not available on AIX, or is it a specific version of the OS that doesn't have it? The doc suggests it is available: https://www.ibm.com/docs/en/aix/7.1?topic=m-madvise-subroutine
Is that specific machine missing a header? Perhaps HAVE_SYS_MMAN_H isn't defined? @simon_tatham

Unless any of you have a way to quickly test a solution, short term should we just #ifndef _AIX the call to madvise?

zibi added a comment.Dec 31 2021, 10:26 AM

@zibi @daltenty Is madvise not available on AIX, or is it a specific version of the OS that doesn't have it? The doc suggests it is available: https://www.ibm.com/docs/en/aix/7.1?topic=m-madvise-subroutine
Is that specific machine missing a header? Perhaps HAVE_SYS_MMAN_H isn't defined? @simon_tatham

Unless any of you have a way to quickly test a solution, short term should we just #ifndef _AIX the call to madvise?

@daltenty, I guess the bots are not on 7.1, right?
FYI, we will have to take the same route on z/OS as Windows did since either madvise nor posix_madvise is available.

Is that specific machine missing a header? Perhaps HAVE_SYS_MMAN_H isn't defined? @simon_tatham

Sorry, I think you might have confused me with someone else? I don't maintain an AIX buildbot, and the last time I used AIX at all was around 2000 and then only very briefly.

daltenty added a comment.EditedJan 4 2022, 7:26 AM

@zibi @daltenty Is madvise not available on AIX, or is it a specific version of the OS that doesn't have it? The doc suggests it is available: https://www.ibm.com/docs/en/aix/7.1?topic=m-madvise-subroutine
Is that specific machine missing a header? Perhaps HAVE_SYS_MMAN_H isn't defined? @simon_tatham

Unless any of you have a way to quickly test a solution, short term should we just #ifndef _AIX the call to madvise?

@zibi @daltenty Is madvise not available on AIX, or is it a specific version of the OS that doesn't have it? The doc suggests it is available: https://www.ibm.com/docs/en/aix/7.1?topic=m-madvise-subroutine

Sorry you're right, the header on AIX does normally have it available under the default _ALL_SOURCE feature test macro. Unfortunately the CMake sets _XOPEN_SOURCE=700 (i.e. XOPEN/POSIX compat) when building LLVM on AIX because of other conflicting symbols when we use _ALL_SOURCE.

I think we should still prefer the posix_madvise solution here for AIX, since it's more portable in general (edit: perhaps not, I see a note for glibc that the semantic is different on Linux with POSIX_MADV_DONTNEED https://man7.org/linux/man-pages/man3/posix_madvise.3.html). For z/OS we can introduce a guard as proposed.