[PATCH] D116366: [Support] Add MemoryBuffer::dontNeedIfMmap

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/Support/Unix/Path.inc:874
+void mapped_file_region::dontNeedImpl() {
+  if (Mapping)
+    ::madvise(Mapping, Size, MADV_DONTNEED);
----------------
aganea wrote:
> Since `MADV_DONTNEED` will drop all written changes to the memory pages, is it worth adding `assert(Mode == mapped_file_region::readonly);` here?
Good idea! Applied.


================
Comment at: llvm/lib/Support/Windows/Path.inc:962
 
+void mapped_file_region::dontNeedImpl() {}
+
----------------
aganea wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116366/new/

https://reviews.llvm.org/D116366