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

[Martin Pelikán via Phabricator via llvm-commits]

pelikan added a comment.

partial review only



================
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);
----------------
I can't see a call to munmap(2).  Does this mean each TEST_F() is run in a separate process?


================
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))
----------------
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.


================
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);
----------------
We might benefit from turning this into a writev(2):

struct iov { iov_vec = {{ExtendsRecord, 8}, {B.Buffer, Extents} }, iov_cnt = 2 };


================
Comment at: compiler-rt/lib/xray/xray_fdr_logging.cc:294
   std::memcpy(TLD.RecordPtr, Event, ReducedEventSize);
+  incrementExtents(MetadataRecSize + EventSize);
   endBufferIfFull();
----------------
I trust it's safe to incrementExtents after the memcpy(3) because this will never run concurrently in multiple threads.


================
Comment at: compiler-rt/lib/xray/xray_fdr_logging.cc:345-350
+      if (TLD.BQ != nullptr) {
+        auto EC = TLD.BQ->releaseBuffer(TLD.Buffer);
+        if (EC != BufferQueue::ErrorCode::Ok)
+          Report("At thread exit, failed to release buffer at %p; error=%s\n",
+                 TLD.Buffer.Buffer, BufferQueue::getErrorString(EC));
+      }
----------------
I think it would improve readability if this indented left a bit, as in:

if (TLD.BQ == nullptr) return;

then the rest.


https://reviews.llvm.org/D39526