[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 and

1. All new code compiled in #if LLVM_ENABLE_THREADS is set so it can be seen where the changes were from this PR.
2. The new PR moves to use madvise() instead of the ad-hoc page referencing code I wrote which should avoid SIGSEGVs if the buffer is deallocated.
3. A new property SerialBackgroundQueue().stopAllWork to be used to stop background workers when there is no further call for them. Usually the background "page-in" threads have completed first but it seems with this troublesome test this is not always the case and buffers stored in the static input file cache are being deallocated while being referenced.

[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.

[aganea]

As a general comment for this PR, we avoid usually #ifdefs in code. I think you can write this entire PR without ever using any #ifdef LLVM_ENABLE_THREADS; either by leaving the code active at all times, or by using existing LLVM APIs that already handle that flag. An example of this is llvm::get_threadpool_strategy which falls back to not using threads when LLVM_ENABLE_THREADS is set. Similarly, ThreadPool does the same thing. Also, all places where we test for if (config->readWorkers) we could also test for llvm_is_multithreaded().

[johnno1962]

@aganea, I've removed all the #if which were intended to show clearly where the new code paths are; In practice you can tell by looking for if (config->readWorkers) anyway. I've added use of llvm_is_multithreaded() instead of another couple of #ifs . The current state of this minor PR should address the failures raised in the other thread but we won't know until it merges whether that is the case. The new SerialBackgroundWorkQueue().stopAllWork property should also avoid processing overrunning.

[DavidSpickett]

This would fix an issue on our buildbot, any chance of getting another round of review so we can eventually merge this?

[DavidSpickett]

Our no threading build appears to be fixed but I got an ASAN failure with that change in a threading build: https://lab.llvm.org/buildbot/#/builders/24/builds/13751

Which "fixed" itself in the next build. So I think this is related to:

The new PR moves to use madvise() instead of the ad-hoc page referencing code I wrote which should avoid SIGSEGVs if the buffer is deallocated.

And therefore I have left my fixes in and disabled the test everywhere for now.

It could be my fault, but I presume you'll start by reverting all my changes anyway in favour of this PR so same result either way.

[DavidSpickett]

Probably seen it before, but just in case:
lld asan log

[johnno1962]

Thank you @DavidSpickett for intervening. I've updated this PR to merge with your changes. The problems with this change (the two PRs) are that a test was added which doesn't exercise a typical usage pattern and results in the intermittent asan failure. This PR should resolve that two ways by moving to use madvise() instead of some hand rolled code on a background thread and by introducing a flag to cause processing on the background thread to finish preventing it overrunning. There is also the change that for a build not including threading the new code will not be included. The final change I included is to make the error when the --read-workers flag is used a warning to be able to pass tests in this case. I've removed the #if NDEBUG conditionals as I was asked at one point to keep conditionals to a minimum and I would rather see the benchmarking code stay in as it is useful for release builds. Remember this is not on the default code path of the linker.

[johnno1962]

👋, I'd really like to see this merged before too long. Is there anything outstanding? I could also look at moving the SerialBackgroundWorkQueue into a common area somewhere while I have a dev/test environment but I think finalising the design for that would likely delay things still further.

[johnno1962]

@drodriguez, are we in a position to merge this? It's definitely better than the code related to this feature currently on main the only reservation being changing the buffers to not be null terminated (which you verified for me ‒ thanks!) I've reversed all my other experiments that might have been more speculative.

[ellishg]

This PR has two accepts, so I think we are good to merge. Although, I always wait a day or two after an accept to merge, in case someone else has something to add. Thanks for checking in!