[PATCH] D26232: [XRay][compiler-rt] XRay Buffer Queue

[Serge Rogatch via llvm-commits]

rSerge added inline comments.


================
Comment at: lib/xray/CMakeLists.txt:9
+
+set(XRAY_FDR_SOURCES
+  xray_buffer_queue.cc)
----------------
Can here be a comment on what FDR is?


================
Comment at: lib/xray/tests/unit/buffer_queue_test.cc:20
+
+TEST(BufferQueueTest, API) { BufferQueue Buffers(getpagesize(), 10); }
+
----------------
Here http://man7.org/linux/man-pages/man2/getpagesize.2.html they say "Portable applications should employ sysconf(_SC_PAGESIZE) instead of getpagesize():".
Also, AFAIK, page sizes can be large (gigabytes). Would this code work then?


================
Comment at: lib/xray/xray_buffer_queue.cc:24
+  for (auto &Buf : Buffers) {
+    void *Tmp = malloc(BufferSize);
+    Buf.Buffer = Tmp;
----------------
Why not to do just 1 `malloc` and then distribute the pointers with offsets?


================
Comment at: lib/xray/xray_buffer_queue.cc:32
+std::error_code BufferQueue::getBuffer(Buffer &Buf) {
+  if (Finalizing.load(std::memory_order_acquire))
+    return std::make_error_code(std::errc::state_not_recoverable);
----------------
Shouldn't this be checked inside the mutex lock?


================
Comment at: lib/xray/xray_buffer_queue.cc:46
+  std::lock_guard<std::mutex> Guard(Mutex);
+  Buffers.push_back(Buf);
+  Buf.Buffer = nullptr;
----------------
You are making it a queue, so the least recently used memory region is popped. This is bad for CPU cache. Why not to make it a stack, so that the most recently used buffer is returned first?
It needs just a change to `push_front` here... and perhaps renaming the data structure.


https://reviews.llvm.org/D26232