[PATCH] D39526: [XRay] Use optimistic logging model for FDR mode

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: compiler-rt/lib/xray/tests/unit/fdr_logging_test.cc:167
+  const char *Contents = static_cast<const char *>(
+      mmap(NULL, Size, PROT_READ, MAP_PRIVATE, Fd, 0));
+  ASSERT_NE(Contents, nullptr);
----------------
pelikan wrote:
> I can't see a call to munmap(2).  Does this mean each TEST_F() is run in a separate process?
Good catch -- it just so happens that the lit test runner does it one test case per run. Fixed to register the map to the `ScopedFileCloserAndDeleter`.


================
Comment at: compiler-rt/lib/xray/xray_fdr_logging.cc:79
+  struct timespec Rem;
+  while (clock_nanosleep(CLOCK_REALTIME, 0, &TS, &Rem) &&
+         (Rem.tv_sec != 0 || Rem.tv_nsec != 0))
----------------
pelikan wrote:
> I'd be inclined to use a semaphore instead but I suspect its overhead would be too high due to MOESI traffic for the memory of its internal count.  Please confirm.
We don't want a semaphore because that will cause synchronisation across threads. This explicitly not require synchronisation, and allow threads to observe some other signalling (atomic) mechanism instead, i.e. whether the buffer queue is finalising.


================
Comment at: compiler-rt/lib/xray/xray_fdr_logging.cc:142-146
+      retryingWriteAll(Fd, reinterpret_cast<char *>(&ExtentsRecord),
+                       reinterpret_cast<char *>(&ExtentsRecord) +
+                           sizeof(MetadataRecord));
       retryingWriteAll(Fd, reinterpret_cast<char *>(B.Buffer),
+                       reinterpret_cast<char *>(B.Buffer) + BufferExtents);
----------------
pelikan wrote:
> We might benefit from turning this into a writev(2):
> 
> struct iov { iov_vec = {{ExtendsRecord, 8}, {B.Buffer, Extents} }, iov_cnt = 2 };
Good idea. Maybe changing the API of `retryingWriteAll` to support a number of ranges might be useful to do. Probably not in this change.


================
Comment at: compiler-rt/lib/xray/xray_fdr_logging_impl.h:316-317
 
-static inline void writeFunctionRecord(int FuncId, uint32_t TSCDelta,
-                                       XRayEntryType EntryType,
-                                       char *&MemPtr) XRAY_NEVER_INSTRUMENT {
+static inline void
+writeFunctionRecord(int FuncId, uint32_t TSCDelta,
+                    XRayEntryType EntryType) XRAY_NEVER_INSTRUMENT {
----------------
pelikan wrote:
> Plus million for this coding style.  Much better than wrapping in the middle of the screen.
Thank clang-format -- this only happened because we removed the `MemPtr` argument.


https://reviews.llvm.org/D39526