[lld][MachO] Follow-up to use madvise() for threaded file page-in.

[johnno1962]

Further to #147134 (comment), switch to use the madvise() api to page in mmap'd files.

[llvmbot]

@llvm/pr-subscribers-llvm-support
@llvm/pr-subscribers-llvm-binary-utilities
@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-macho

Author: John Holdsworth (johnno1962)

Changes

Further to #147134 (comment), switch to use the madvise() api to page in mmap'd files.

---

Full diff: https://github.com/llvm/llvm-project/pull/157917.diff

1 Files Affected:

• (modified) lld/MachO/Driver.cpp (+7-8)

diff --git a/lld/MachO/Driver.cpp b/lld/MachO/Driver.cpp
index 3db638e1ead96..2635c82a53448 100644
--- a/lld/MachO/Driver.cpp
+++ b/lld/MachO/Driver.cpp
@@ -53,6 +53,8 @@
 #include "llvm/TextAPI/Architecture.h"
 #include "llvm/TextAPI/PackedVersion.h"
 
+#include <sys/mman.h>
+
 using namespace llvm;
 using namespace llvm::MachO;
 using namespace llvm::object;
@@ -334,11 +336,10 @@ class SerialBackgroundQueue {
 // This code forces the page-ins on multiple threads so
 // the process is not stalled waiting on disk buffer i/o.
 void multiThreadedPageInBackground(DeferredFiles &deferred) {
-  static const size_t pageSize = Process::getPageSizeEstimate();
   static const size_t largeArchive = 10 * 1024 * 1024;
 #ifndef NDEBUG
   using namespace std::chrono;
-  std::atomic_int numDeferedFilesTouched = 0;
+  std::atomic_int numDeferedFilesAdvised = 0;
   static std::atomic_uint64_t totalBytes = 0;
   auto t0 = high_resolution_clock::now();
 #endif
@@ -349,13 +350,11 @@ void multiThreadedPageInBackground(DeferredFiles &deferred) {
       return;
 #ifndef NDEBUG
     totalBytes += buff.size();
-    numDeferedFilesTouched += 1;
+    numDeferedFilesAdvised += 1;
 #endif
 
-    // Reference all file's mmap'd pages to load them into memory.
-    for (const char *page = buff.data(), *end = page + buff.size(); page < end;
-         page += pageSize)
-      LLVM_ATTRIBUTE_UNUSED volatile char t = *page;
+    // Advise that mmap'd files should be loaded into memory.
+    madvise((void *)buff.data(), buff.size(), MADV_WILLNEED);
   };
 #if LLVM_ENABLE_THREADS
   { // Create scope for waiting for the taskGroup
@@ -376,7 +375,7 @@ void multiThreadedPageInBackground(DeferredFiles &deferred) {
   auto dt = high_resolution_clock::now() - t0;
   if (Process::GetEnv("LLD_MULTI_THREAD_PAGE"))
     llvm::dbgs() << "multiThreadedPageIn " << totalBytes << "/"
-                 << numDeferedFilesTouched << "/" << deferred.size() << "/"
+                 << numDeferedFilesAdvised << "/" << deferred.size() << "/"
                  << duration_cast<milliseconds>(dt).count() / 1000. << "\n";
 #endif
 }

[aganea]

LGTM.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[johnno1962]

I've included changes to two additional files to counter the performance regression of 2 seconds from 85cd3d9 for the linker when using the --worker-threads option due to not mmap()ing many input files. Binary object and archive files don't need to be null terminated. This seemed the least intrusive change.

[drodriguez]

've included changes to two additional files to counter the performance regression of 2 seconds from 85cd3d9 for the linker when using the --worker-threads option due to not mmap()ing many input files. Binary object and archive files don't need to be null terminated. This seemed the least intrusive change.

The one in InputFiles.cpp is also used for tbd files, if I am not mistaken. Those are textual files (YAML and recently JSON), and I am not sure if the parser might be tripped from the missing \0 towards the end if the situation described in 85cd3d9 happens.

[johnno1962]

@drodriguez I wondered about .tbd files. Basically, the new code in the commit I mention, prevents most input files from using mmap() to solve an obscure clang problem on Linux. I'm open to suggestions about what to do about this.

Edit:

prevents most input files from using mmap()

This is an incorrect assumption see below..

[ellishg]

Not sure how I feel about leaking object file logic into MemoryBuffer.cpp. Could we just use RequiresNullTerminator = false in getFile() for object files (or however this code is called)?

[johnno1962]

I know, this version is awful but at least the fix is located close to the change that caused the problem. Have a look at the commit to see the previous version which passed in RequiresNullTerminator = false.

[johnno1962]

Any ideas how we should handle this @zygoloid?

[zygoloid]

Why would we be setting RequiresNullTerminator when mapping a .o or .a file? That sounds like a bug in the caller to me.

[drodriguez]

@zygoloid do you remember why you did not limit the solution in 85cd3d9 to just Linux if only that platform was the one exhibiting the bug. It has performance implications, which other platforms might not need to pay if they don't have the same bug.

[zygoloid]

@drodriguez I wondered about .tbd files. Basically, the new code in the commit I mention, prevents most input files from using mmap() to solve an obscure clang problem on Linux.

Can you explain how it prevents most input files from using mmap? I would expect that for most input files either the file is binary (in which case RequiresNullTerminator should be false), or there is a null terminator produced by mmap (in which case you get to use mmap), or you're hitting an OS bug like the Linux one.

[johnno1962]

The default value for RequiresNullTerminator in the header for MemoryBuffer::getFile(...) is true when not specified.

[zygoloid]

The default value for RequiresNullTerminator in the header for MemoryBuffer::getFile(...) is true when not specified.

Can we fix that? That really seems like the wrong default, given that -- as we've discovered -- guaranteeing a null terminator is not free. Failing to provide the requested guarantee in some cases seems like the wrong answer.

[drodriguez]

@zygoloid I found the existing discussion about why not only Linux in the comments of #152595. Makes sense.

The problem is that the default of RequiresNullTerminator makes all the simple invocations to getFile, even if being binaries, fall onto the slow path of not being mmapped, this is a change from the status quo before #152595. Trying to modify the default of RequiresNullTerminator will involve going through every callsite of getFile and figure out if the default was being used intentionally, or the default was just accidentally working.

[zygoloid]

@zygoloid I found the existing discussion about why not only Linux in the comments of #152595. Makes sense.

The problem is that the default of RequiresNullTerminator makes all the simple invocations to getFile, even if being binaries, fall onto the slow path of not being mmapped

Why? They'd only take the slow path if their size is a multiple of the page size or they hit an OS bug.

[johnno1962]

Object files are a multiple of the page size no? And what about source files that happen to have a length which is a multiple of the page size?

[zygoloid]

Object files are a multiple of the page size no?

If so, any code opening them as a memory buffer really shouldn't be passing RequiresNullTerminator = true, or it will never use mmap, regardless of #152595. RequiresNullTerminator = true is the wrong default.

[drodriguez]

@zygoloid I cannot find an online link, but for example, in my macOS 15.6.1, man mmap has the following text:

Any extension beyond the end of the mapped object will be zero-filled.

And there's no BUGS section in there. I think this workaround might not me needed for macOS, and maybe by reading the buffer end byte, the access pattern has changed enough to have performance implications. It might be creating a page fault, if the file size is the right one to fall in a page boundary.

@johnno1962 : I have checked the YAML/JSON parsers for the tbd and they seem to check for the length of the files and not blindly searching for the end of the string. I cannot say those are the only textual files that are processed (linker scripts files and similar might be accepted?). If you have executed the test suite and nothing pops, maybe this is correct, but the situation described in #152595 is not something that can be replicated easily in the tests, so this might be introducing problems.

[johnno1962]

FWIW Object files seem to have a length which is a multiple of 8.

[zygoloid]

@zygoloid I cannot find an online link, but for example, in my macOS 15.6.1, man mmap has the following text:

Any extension beyond the end of the mapped object will be zero-filled.

And there's no BUGS section in there. I think this workaround might not me needed for macOS, and maybe by reading the buffer end byte, the access pattern has changed enough to have performance implications. It might be creating a page fault, if the file size is the right one to fall in a page boundary.

It may still be needed in some cases even without the Linux bug. For example, if another process (say, a text editor) mmaps the file and writes into the slack bytes, then clang is executed while that other process still has the file open, clang's mmap might see those non-zero bytes, because it will share the pages. It should be easy enough to write a test program to see if that happens in practice. (Does the OS zero the slack bytes when the last page is first mapped, or each time it's mapped?)

But to reiterate, the problem here is that people are incorrectly passing RequiresNullTerminator = true. We should fix that. If you want to also restrict the workaround for bad mmaps to apply only on Linux, I'm fine with that (that's what my original PR did), but that's not really getting to the heart of the problem, and you'll still have slow fallbacks to read for files that happen to be a "bad" size.

[johnno1962]

So I guess the current version of this PR is as good as it gets for now. Probably, the default argument should be removed from MemoryBuffer::getFile altogether. It is referenced in 245 places (about half tests). There's also the preceding argument isText which one wonders if it is similar enough to roll them into one. The signature for getFile is currently:

  static ErrorOr<std::unique_ptr<MemoryBuffer>>
  getFile(const Twine &Filename, bool IsText = false,
          bool RequiresNullTerminator = true, bool IsVolatile = false,
          std::optional<Align> Alignment = std::nullopt);

ErrorOr<std::unique_ptr<MemoryBuffer>>
MemoryBuffer::getFile(const Twine &Filename, bool IsText,
                      bool RequiresNullTerminator, bool IsVolatile,
                      std::optional<Align> Alignment) {
  return getFileAux<MemoryBuffer>(Filename, /*MapSize=*/-1, /*Offset=*/0,
                                  IsText, RequiresNullTerminator, IsVolatile,
                                  Alignment);
}

template <typename MB>
static ErrorOr<std::unique_ptr<MB>>
getFileAux(const Twine &Filename, uint64_t MapSize, uint64_t Offset,
           bool IsText, bool RequiresNullTerminator, bool IsVolatile,
           std::optional<Align> Alignment) {
  Expected<sys::fs::file_t> FDOrErr = sys::fs::openNativeFileForRead(
      Filename, IsText ? sys::fs::OF_TextWithCRLF : sys::fs::OF_None);

Looks like this would be a difficult signature to modify as it is also referenced (using the default) 54 times in the swift project. One migration path would be to create a new API without the default e.g. getFileBuffer and people can slowly switch to that.

[johnno1962]

@zygoloid, just an update on what I'm trying with recent commits. I'm seeking to verify the follow change to your change:

@@ -506,7 +509,7 @@ getOpenFileImpl(sys::fs::file_t FD, const Twine &Filename, uint64_t FileSize,
       // from the page cache that are not properly filled with trailing zeroes,
       // if some prior user of the page wrote non-zero bytes. Detect this and
       // don't use mmap in that case.
-      if (!RequiresNullTerminator || *Result->getBufferEnd() == '\0')
+      if (!IsText || !RequiresNullTerminator || *Result->getBufferEnd() == '\0')
         return std::move(Result);
     }
   }

... along with some other minor changes to the signatures of functions internal to MemoryBuffer.cpp to marshal the IsText value through to getOpenFileImpl(). The difference is IsText has a default argument value of false whereas the default value for RequiresNullTerminator is true when you don't specify a value which seems to be the common case. I appreciate this is a minor fib for the code but there don't seem to be many other solutions given the constraints of a well used entry point. I don't imagine many source buffers that require null termination are not also being specified as text in practice and if that assumption didn't hold the worst case is to revert to the behaviour before Aug 13th.

How would you feel about me making this change? There were Ci problems but they fixed themselves.

If you're wondering how I'm testing/proving this change I have a trial link of the largest .dylib in Chrome which always takes 14 seconds using --read-workers=20 after 85cd3d9 and 12 seconds before or with any of the last six defensive commits on a freshly rebooted machine. An excerpt from the bisection I did to find the PR.

Thu Aug 14 13:47:48 b62b65a95f2b5e79e90f3f957e7a52ec50c5fe31 0m14.374s
Thu Aug 14 12:08:28 71b066e3a2512d582e34a0b5257e12b1177d4bcc 0m14.552s 
Wed Aug 13 13:58:52 da422daea9c31ad00ff9e2d8a401e404c3ed58e1 0m14.445s
Wed Aug 13 12:39:25 85cd3d98686c47d015dbcc17f1f7d0714b00e172 0m14.537s 0m14.254s
Wed Aug 13 19:37:27 36d31b0c008b2716329b5c9990f583decf919819 0m12.415s
Wed Aug 13 12:31:55 088b8ffca10700475e98ccebeb54622967e28397 0m12.333s
Wed Aug 13 14:24:47 dedc5916a53c160b3c1bac163fd6d92bc5882e21 0m12.201s

A few recent commits:
Fri Sep 12 16:56:01 f2654ae6e1bb7aa78a16badb1bcc9d6f1f824676 0m12.314s
Thu Sep 11 17:19:19 e31984b250c3fdd7980efdfe5683c155b409d826 0m12.061s
Thu Sep 11 08:53:49 298380eb921bae4f2507aebf5b59611d76a41117 0m14.481s

P.S. I worked out where the missing 2 seconds are actually going. It's not because i/o is dropping down from mmap() to a file read at all but the very act of checking for the terminating byte waits for a synchronous page read, single threaded for the 22,000 input files being opened in the example link. .1ms per random access read from secondary storage seems credible.
This change is as slow 0m14.362s (if you uncomment the if nothing is ever printed):

-      if (!IsText || *Result->getBufferEnd() == '\0')
+      static int i;
+      if (*Result->getBufferEnd() == '\0')
+        i++;
+//      if (!RequiresNullTerminator || *Result->getBufferEnd() == '\0')
         return std::move(Result);
+      printf("%d %lx %s\n", i, FileSize, Filename.str().c_str());