tracing v2: Add TraceRingBuffer class.

components/tracing/core/trace_ring_buffer.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/tracing/core/trace_ring_buffer.h"
+
+#include "base/threading/platform_thread.h"
+
+namespace tracing {
+namespace v2 {
+
+TraceRingBuffer::TraceRingBuffer(uint8_t* begin, size_t size)
+    : num_chunks_(size / Chunk::kSize), current_chunk_idx_(0) {
+  DCHECK_GE(size, Chunk::kSize);

[petrcermak, 2016/06/28 10:22:29]

maybe check DCHECK_GT(num_chunks_, 0) since that's what you're really after?

[Primiano Tucci (use gerrit), 2016/06/28 11:30:14]

makes sense, thanks. Done

+  DCHECK_EQ(0ul, reinterpret_cast<uintptr_t>(begin) % sizeof(uintptr_t));
+  chunks_.reset(new Chunk[num_chunks_]);
+  uint8_t* chunk_begin = begin;
+  for (size_t i = 0; i < num_chunks_; ++i) {
+    chunks_[i].Initialize(chunk_begin);
+    chunk_begin = chunk_begin + Chunk::kSize;

[petrcermak, 2016/06/28 10:22:29]

chunk_begin += Chunk::kSize

alternatively, use multiplication (it makes the code more readable in my
opinion):

for (size_t i = 0; i < num_chunks_; ++i)
  chunks_[i].Initialize(chunk_begin + i * Chunk::kSize)

[Primiano Tucci (use gerrit), 2016/06/28 11:30:14]

Oh definitely. The only reason why it is that way is because that was the result
of a lot of re-formattings on my side, and didn't realize that I ended up with a
+= at the end.

+  }
+}
+
+TraceRingBuffer::~TraceRingBuffer() {}
+
+TraceRingBuffer::Chunk* TraceRingBuffer::TakeChunk() {
+  base::AutoLock lock(lock_);
+  DCHECK_GT(num_chunks_, 0ul);
+  DCHECK_LT(current_chunk_idx_, num_chunks_);
+  Chunk* chunk = nullptr;

[petrcermak, 2016/06/28 10:22:29]

Why don't you declare this inside the loop on line 32???

[Primiano Tucci (use gerrit), 2016/06/28 11:30:14]

Same as above, historical evolution of this code. That's why I needed a review
:)
Thanks.

+  for (size_t i = 0; i < num_chunks_; ++i) {
+    chunk = &chunks_[current_chunk_idx_];
+    current_chunk_idx_ = (current_chunk_idx_ + 1) % num_chunks_;
+    if (!chunk->is_owned()) {
+      chunk->Clear();
+      chunk->set_owner(base::PlatformThread::CurrentId());
+      return chunk;
+    }
+  }
+
+  // Bankrupcy: there are more threads than chunks. All chunks were on flight.

[petrcermak, 2016/06/28 10:22:29]

nit: s/on/in/

[Primiano Tucci (use gerrit), 2016/06/28 11:30:14]

Done.

+  if (!bankrupcy_chunk_storage_) {
+    bankrupcy_chunk_storage_.reset(new uint8_t[Chunk::kSize]);
+    uint8_t* const begin = &bankrupcy_chunk_storage_.get()[0];

[petrcermak, 2016/06/28 10:22:29]

Opinion: I don't think that naming this "begin" makes the code any more
readable, so I'd just fold the variable.

[Primiano Tucci (use gerrit), 2016/06/28 11:30:14]

Oh, as above. Before there was a "begin" and "end", which made more sense :)
Agree, now that folding is the way.

+    bankrupcy_chunk_.Initialize(begin);
+  }
+  bankrupcy_chunk_.Clear();
+  return &bankrupcy_chunk_;
+}
+
+void TraceRingBuffer::ReturnChunk(TraceRingBuffer::Chunk* chunk,
+                                  uint32_t used_size) {
+  base::AutoLock lock(lock_);

[petrcermak, 2016/06/28 10:22:29]

Would it make sense to DCHECK that the chunk is actually in |chunks_| or would
it hurt performance too much?

[Primiano Tucci (use gerrit), 2016/06/28 11:30:14]

Hmm that would make the debug (or DCHECK_ALWAYS_ON) code really slow, as I
expect we'll have quite some chunks and the dcheck should have to iterate over
them.
This should be quite unlikely to happen, as the chunk ctor is private IIRC and
the only way to get a chunk is via TakeChunk.

+  chunk->set_used_size(used_size);
+  chunk->clear_owner();
+}
+
+TraceRingBuffer::Chunk::Chunk()
+    : begin_(nullptr), owner_(base::kInvalidThreadId) {}
+
+TraceRingBuffer::Chunk::~Chunk() {}
+
+void TraceRingBuffer::Chunk::Clear() {

[petrcermak, 2016/06/28 10:22:29]

Shouldn't this also clear owner?

[Primiano Tucci (use gerrit), 2016/06/28 11:30:14]

No this one is deliberate. I mean whether conceptually it should happen is
borderline.
In practice the owner is always reset by the caller after the clear so this
would end up in rendundant code  as we'd clear and immediately reset the owner.

+  set_used_size(0);
+}
+
+void TraceRingBuffer::Chunk::Initialize(uint8_t* begin) {
+  DCHECK_EQ(0ul, reinterpret_cast<uintptr_t>(begin) % sizeof(uintptr_t));
+  begin_ = begin;
+}
+
+}  // namespace v2
+}  // namespace tracing