[PATCH] D69575: Improve module.pcm lock file performance on machines with high core counts

[Michael Spencer via Phabricator via llvm-commits]

Bigcheese accepted this revision.
Bigcheese added a comment.
This revision is now accepted and ready to land.

A few minor points, but with those fixed this looks good to me. Thanks for the patch!



================
Comment at: llvm/lib/Support/LockFileManager.cpp:49
+// Could build a better availability check for kevents
+#if defined(__APPLE__) && defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && (__MAC_OS_X_VERSION_MIN_REQUIRED > 1030)
+#define HAVE_KEVENTS 1
----------------
80 column limit.


================
Comment at: llvm/lib/Support/LockFileManager.cpp:317
+  // If we don't have an event-based method to wait for the lock file,
+  // implement randomized exponential backoff, similar to Ethernet collision algorithm
+  // This improves performance on machines with high core counts
----------------
80 col


================
Comment at: llvm/lib/Support/LockFileManager.cpp:339-340
+    // Supported on macOS and various BSD UNIX.
+    int KernelQueue;
+    int LockFileDescriptor;
+    struct kevent EventsToMonitor[1];
----------------
These declarations should be sunk to their smallest enclosing scope. 


================
Comment at: llvm/lib/Support/LockFileManager.cpp:341-342
+    int LockFileDescriptor;
+    struct kevent EventsToMonitor[1];
+    struct kevent EventData[1];
+    struct timespec Timeout;
----------------
Pretty sure these don't need to be arrays, you should just be able to take the address of a `kevent`.


================
Comment at: llvm/lib/Support/LockFileManager.cpp:341-343
+    struct kevent EventsToMonitor[1];
+    struct kevent EventData[1];
+    struct timespec Timeout;
----------------
Bigcheese wrote:
> Pretty sure these don't need to be arrays, you should just be able to take the address of a `kevent`.
These don't need the `struct` keyword in C++, although it's already used elsewhere in the code.


================
Comment at: llvm/lib/Support/LockFileManager.cpp:353
+
+        EV_SET( &EventsToMonitor[0], LockFileDescriptor, EVFILT_VNODE, EV_ADD | EV_CLEAR, NOTE_DELETE, 0, NULL);
+        
----------------
No space before `&` here, and 80 col.


================
Comment at: llvm/lib/Support/LockFileManager.cpp:370
+      unsigned long WaitDurationMS = MinWaitDurationMS*Distribution(Engine);
+      std::this_thread::sleep_for (std::chrono::milliseconds(WaitDurationMS));
+    }
----------------
No space before `(` here.


================
Comment at: llvm/lib/Support/LockFileManager.cpp:390
+    
+    ElapsedTimeSeconds = std::chrono::duration_cast<std::chrono::seconds>(std::chrono::steady_clock::now() - StartTime).count();
+    
----------------
80 col


================
Comment at: llvm/lib/Support/LockFileManager.cpp:392
+    
+  } while ( ElapsedTimeSeconds < MaxTotalSeconds );
 
----------------
Remove extra spaces after `(` and before `)`.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D69575