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

[Dean Michael Berris via llvm-commits]

dberris added inline comments.


================
Comment at: lib/xray/tests/unit/buffer_queue_test.cc:20
+
+TEST(BufferQueueTest, API) { BufferQueue Buffers(getpagesize(), 10); }
+
----------------
rSerge wrote:
> 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?
This is really just a test, the page size is a convenient way of getting large-ish values. :)


================
Comment at: lib/xray/xray_buffer_queue.cc:24
+  for (auto &Buf : Buffers) {
+    void *Tmp = malloc(BufferSize);
+    Buf.Buffer = Tmp;
----------------
rSerge wrote:
> Why not to do just 1 `malloc` and then distribute the pointers with offsets?
The idea is to have differentiated buffers that can be treated as individual thunks of memory. It also allows us to check for when malloc fails.


================
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);
----------------
rSerge wrote:
> Shouldn't this be checked inside the mutex lock?
Nope, `Finalizing` is atomic, and is already synchronised -- so we avoid locking the mutex when the BufferQueue is already finalizing.


================
Comment at: lib/xray/xray_buffer_queue.cc:46
+  std::lock_guard<std::mutex> Guard(Mutex);
+  Buffers.push_back(Buf);
+  Buf.Buffer = nullptr;
----------------
rSerge wrote:
> 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.
The point of the queue is so that we can ensure a temporal bound, i.e. we can keep around buffers that have been filled before. We explicitly want to actually keep the data around in the buffers, so that operations that have happened in the past are kept.

This is a key concept that the flight data recorder mode described in the whitepaper actually requires. Otherwise, if we made this a stack, then only the most recent operations will ever be kept (as opposed to a running log of things that have happened in the past).


https://reviews.llvm.org/D26232