tracing v2: Add ScatteredStreamWriter

components/tracing/core/scattered_stream_writer.cc

Index: components/tracing/core/scattered_stream_writer.cc
diff --git a/components/tracing/core/scattered_stream_writer.cc b/components/tracing/core/scattered_stream_writer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..756710d13ec2bf6be016adb84630728601e653ee
--- /dev/null
+++ b/components/tracing/core/scattered_stream_writer.cc
@@ -0,0 +1,75 @@
+// 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/scattered_stream_writer.h"
+
+#include <string.h>
+
+#include "base/logging.h"
+
+namespace tracing {
+namespace v2 {
+
+ScatteredStreamWriter::ScatteredStreamWriter(Delegate* delegate)
+    : delegate_(delegate),
+      cur_range_({nullptr, nullptr}),
+      write_ptr_(nullptr) {}
+
+ScatteredStreamWriter::~ScatteredStreamWriter() {}
+
+void ScatteredStreamWriter::Reset(ContiguousMemoryRange range) {
+  cur_range_ = range;
+  write_ptr_ = range.begin;
+  DCHECK_LT(write_ptr_, cur_range_.end);
+}
+
+void ScatteredStreamWriter::Extend() {
+  Reset(delegate_->GetNewBuffer());
+}
+
+void ScatteredStreamWriter::WriteByte(uint8_t value) {
+  if (write_ptr_ >= cur_range_.end)

[petrcermak, 2016/06/29 10:27:48]

You do this in 2 places. Assuming the compiler inlines it (performance), I think
it make sense to have an ExtendIfNecessary() method instead.

[Primiano Tucci (use gerrit), 2016/06/30 12:17:34]

I thought to that. Is not about perf and inlining. I just think that this is
more readable having the explicit if. otherwise in order to figure out what
ExpandIfNecessary does your eyes have to hop on 3 places. (ExpandIfNecessary ->
Expand() -> Reset(NewBuffer)).
Since there are only 2 cases, code duplication is not that much w.r.t. the
resulting unreadability. More practically known as "Rule of Three":
https://en.wikipedia.org/wiki/Rule_of_three_(computer_programming)

+    Extend();
+  *write_ptr_++ = value;
+}
+
+void ScatteredStreamWriter::WriteBytes(const uint8_t* src, size_t size) {
+  uint8_t* const end = write_ptr_ + size;
+  if (end <= cur_range_.end) {

[petrcermak, 2016/06/29 10:27:48]

You could make this method simpler and avoid recursion by simply using the loop:

while (size > 0) {
  if (write_ptr_ >= cur_range_.end)
    Extend();
  const size_t burst_size = std::min(bytes_available(), size);
  memcpy(write_ptr, src, burst_size);
  size -= burst_size;
  src += burst_size;
}

[Primiano Tucci (use gerrit), 2016/06/30 12:17:34]

I know, but that would have 3 if-s in the fastpath (one hidden in the std::min),
mine has one. It's just about perf and helping the branch predictor. This path
will  really be hit a lot, perf shows this the hotspot of tracing
(unsurprisingly).
slighly reformatted this, the else block wasn't really required after the else.

+    // Fast-path, the buffer fits into the current contiguous range.
+    // TODO(primiano): perf optimization, this is a tracing hot path. The
+    // compiler can make strong optimization on memcpy if the size arg is a
+    // constexpr. Make a templated variant of this for fixed-size writes.
+    memcpy(write_ptr_, src, size);
+    write_ptr_ = end;
+    return;

[petrcermak, 2016/06/29 10:27:48]

you use both return and else. only one is necessary

[Primiano Tucci (use gerrit), 2016/06/30 12:17:34]

right. done.

+  } else {
+    // Slow path, scatter the writes.
+    size_t bytes_left = size;
+    while (bytes_left > 0) {
+      if (write_ptr_ >= cur_range_.end)
+        Extend();
+      const size_t burst_size = std::min(bytes_available(), bytes_left);
+      WriteBytes(src, burst_size);
+      bytes_left -= burst_size;
+      src += burst_size;
+    }
+  }
+}
+
+// TODO(primiano): perf optimization: I suspect that at the end this will always
+// be called with |size| == 4, in which case we might just hardcode it.
+ContiguousMemoryRange ScatteredStreamWriter::ReserveBytes(size_t size) {
+  if (write_ptr_ + size > cur_range_.end)
+    Extend();
+  uint8_t* begin = write_ptr_;
+  write_ptr_ += size;
+#ifndef NDEBUG
+  memset(begin, '\xFF', size);
+#endif
+  return {begin, begin + size};

[petrcermak, 2016/06/29 10:27:48]

You need to check that |size| is not greater than the size of the chunk.
Otherwise, you've just returned an invalid range of memory!!!

[Primiano Tucci (use gerrit), 2016/06/30 12:17:34]

Correct, that was just an assumption, perhaps too implicit and undocumented.
Added a DCHECK, but will remove later it once this becomes a fixed-size function
with no size argument, this will be a dcheck in a fast path that will make debug
uber slow..

+}
+
+}  // namespace v2
+}  // namespace tracing