tracing v2: Add base class for zero-copy protobuf (ProtoZero)

components/tracing/core/proto_zero_message.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/proto_zero_message.h"
+
+#include <string.h>
+
+#include "build/build_config.h"
+#include "components/tracing/core/proto_utils.h"
+
+#if !defined(ARCH_CPU_LITTLE_ENDIAN)
+// The memcpy() for float and double below needs to be adjusted if we want to
+// support big endian CPUs. There doesn't seem to be a compelling need today.
+#error big-endian CPUs not supported by this translation unit.
+#endif
+
+namespace tracing {
+namespace v2 {
+
+ProtoZeroMessage::ProtoZeroMessage() {
+  // Do add any code here, use the Reset() method below.

[petrcermak, 2016/07/12 10:18:46]

s/Do/Don't/ ?

[petrcermak, 2016/07/12 10:18:46]

nit: s/below/below instead/

[Primiano Tucci (use gerrit), 2016/07/12 17:25:16]

Done.

[Primiano Tucci (use gerrit), 2016/07/12 17:25:16]

EHm, yeah :)

+  // Ctor and Dtor of ProtoZeroMessage are never called, with the exeception
+  // of root (non-nested) messages. Nested messages are allocated into the

[petrcermak, 2016/07/12 10:18:46]

nit: s/into/in/ (I've never heard "allocated into")

[Primiano Tucci (use gerrit), 2016/07/12 17:25:16]

In my mind "allocated into" is about the actual storage (e.g., allocated into X
pages, allocated into that buffer), while "allocated in" is more about the
context (allocated in foobar.cc, allocated in MethodName())

[petrcermak, 2016/07/12 17:41:17]

Acknowledged.

+  // |nested_messages_arena_| and implictly destroyed when the arena of the
+  // root message goes away. This is fine as long as all the fields are PODs,
+  // hence the static_assert below.
+  static_assert(base::is_trivially_destructible<ProtoZeroMessage>::value,
+                "ProtoZeroMessageIsTriviallyDestructible");
+
+  static_assert(
+      sizeof(ProtoZeroMessage::nested_messages_arena_) >=
+          kMaxNestingDepth * (sizeof(ProtoZeroMessage) -

[alph, 2016/07/12 00:20:42]

nit: offsetof

[Primiano Tucci (use gerrit), 2016/07/12 10:15:32]

Hmm if you don't mind I'd leave this as it is. The final math would be the same,
but I feel offsetof would make it a bit more unreadable.
What I really want to say here is: the arena should be big enough to contain
kMax elements, without counting the arena itself in nested elements.

+                              sizeof(ProtoZeroMessage::nested_messages_arena_)),
+      "NestedMessagesAreaTooSmall");
+}
+
+// This method is called to initialize both root and nested messages.
+void ProtoZeroMessage::Reset(ScatteredStreamWriter* stream_writer) {
+  stream_writer_ = stream_writer;
+  size_ = 0;
+  size_field_.reset();
+  size_already_written_ = 0;
+  nested_message_ = nullptr;
+  nesting_depth_ = 0;
+#if DCHECK_IS_ON()
+  sealed_ = false;

[petrcermak, 2016/07/12 10:18:47]

You don't actually DCHECK this anywhere. Shouldn't it be DCHECK'ed in every
single Append* method?

[Primiano Tucci (use gerrit), 2016/07/12 17:25:16]

EPIC FAIL. good spot, yes. :P
Just WriteToStream and Finalize is enough.

+#endif
+}
+
+void ProtoZeroMessage::AppendVarIntU64(uint32_t field_id, uint64_t value) {
+  if (nested_message_)

[petrcermak, 2016/07/12 10:18:47]

You do this a lot. How about EndNestedMessageIfNecessary() instead?

[Primiano Tucci (use gerrit), 2016/07/12 17:25:16]

I knew that you were going to suggest it. I though to that but IMHO is more
readable having the full line.

[petrcermak, 2016/07/12 17:41:17]

And I knew this was going to be your reply :-P

+    EndNestedMessage();
+
+  uint8_t data[proto::GetMaxVarIntEncodedSize<uint32_t>() +
+               proto::GetMaxVarIntEncodedSize<uint64_t>()];
+  uint8_t* data_end;
+  data_end = proto::WriteVarIntU32(proto::MakeTagVarInt(field_id), data);
+  data_end = proto::WriteVarIntU64(value, data_end);
+  WriteToStream(data, data_end);
+}
+
+void ProtoZeroMessage::AppendVarIntU32(uint32_t field_id, uint32_t value) {
+  // TODO(kraynov): this could be perf-optimized. See http://crbug.com/624311 .

[petrcermak, 2016/07/12 10:18:46]

nit: remove unnecessary space before period.

[Primiano Tucci (use gerrit), 2016/07/12 17:25:16]

It's on purpose, so that the clickable link does not contain the dot.
It's a "finesse", essentially :)

[petrcermak, 2016/07/12 17:41:17]

Acknowledged.

+  AppendVarIntU64(field_id, value);
+}
+
+void ProtoZeroMessage::AppendFloat(uint32_t field_id, float value) {
+  if (nested_message_)
+    EndNestedMessage();
+
+  uint8_t data[proto::GetMaxVarIntEncodedSize<uint32_t>() + sizeof(value)];
+  uint8_t* data_end;
+  data_end = proto::WriteVarIntU32(proto::MakeTagFixed32(field_id), data);
+  memcpy(data_end, &value, sizeof(value));
+  data_end += sizeof(value);
+  WriteToStream(data, data_end);
+}
+
+void ProtoZeroMessage::AppendDouble(uint32_t field_id, double value) {
+  if (nested_message_)
+    EndNestedMessage();
+
+  uint8_t data[proto::GetMaxVarIntEncodedSize<uint32_t>() + sizeof(value)];
+  uint8_t* data_end;
+  data_end = proto::WriteVarIntU32(proto::MakeTagFixed64(field_id), data);
+  memcpy(data_end, &value, sizeof(value));
+  data_end += sizeof(value);
+  WriteToStream(data, data_end);
+}
+
+void ProtoZeroMessage::AppendString(uint32_t field_id, const char* str) {
+  AppendBytes(field_id, str, strlen(str));
+}
+
+void ProtoZeroMessage::AppendBytes(uint32_t field_id,
+                                   const void* src,
+                                   size_t size) {
+  if (nested_message_)
+    EndNestedMessage();
+
+  // Write the proto preamble (field id, type and length of the buffer).
+  DCHECK_LT(size, proto::kMaxMessageLength);
+  uint8_t data[2 * proto::GetMaxVarIntEncodedSize<uint32_t>()];
+  uint8_t* data_end =

[petrcermak, 2016/07/12 10:18:47]

In most other methods you declare and assign to |data_end| separately.

[Primiano Tucci (use gerrit), 2016/07/12 17:25:16]

I knew also this :P
There is a reason. In the other methods the uint8_t* data_end = bla would wrap
over two lines. Since I have to spend two lines I find more legible having the
declaration and assignment being separate (and the assignment fits in one line).

In this case however even the assignment itself won't fit on a single line, so
why bothering with the separate declaration?

[petrcermak, 2016/07/12 17:41:17]

Acknowledged.

+      proto::WriteVarIntU32(proto::MakeTagLengthDelimited(field_id), data);
+  data_end = proto::WriteVarIntU32(static_cast<uint32_t>(size), data_end);
+  WriteToStream(data, data_end);
+  const uint8_t* src_u8 = reinterpret_cast<const uint8_t*>(src);
+  WriteToStream(src_u8, src_u8 + size);
+}
+
+size_t ProtoZeroMessage::Finalize() {
+  if (nested_message_)
+    EndNestedMessage();
+
+  if (size_field_.is_valid()) {
+    // Write the length of the nested message a posteriori, using a leading-zero
+    // redundant varint encoding.
+    DCHECK_LT(size_, proto::kMaxMessageLength);
+    DCHECK_EQ(proto::kMessageLengthFieldSize, size_field_.size());
+    proto::WriteRedundantVarIntU32<proto::kMessageLengthFieldSize>(
+        static_cast<uint32_t>(size_ - size_already_written_),
+        size_field_.begin);
+    size_field_.reset();
+  }
+
+#if DCHECK_IS_ON()
+  sealed_ = true;
+#endif
+
+  return size_;
+}
+
+void ProtoZeroMessage::BeginNestedMessageInternal(uint32_t field_id,
+                                                  ProtoZeroMessage* message) {
+  if (nested_message_)
+    EndNestedMessage();
+
+  // Write the proto preamble for the nested message.
+  uint8_t data[proto::GetMaxVarIntEncodedSize<uint32_t>()];
+  uint8_t* data_end =

[petrcermak, 2016/07/12 10:18:46]

ditto (separate declaration and assignment)

[Primiano Tucci (use gerrit), 2016/07/12 17:25:16]

ditto (assignment doesn't fit in one line)

[petrcermak, 2016/07/12 17:41:17]

Acknowledged.

+      proto::WriteVarIntU32(proto::MakeTagLengthDelimited(field_id), data);
+  WriteToStream(data, data_end);
+
+  message->Reset(stream_writer_);
+  CHECK_LT(nesting_depth_, kMaxNestingDepth);
+  message->nesting_depth_ = nesting_depth_ + 1;
+
+  // The length of the nested message cannot be known upfront. So right now
+  // just reserve the bytes to encode the size after the nested message is done.
+  message->set_size_field(
+      stream_writer_->ReserveBytes(proto::kMessageLengthFieldSize));
+  size_ += proto::kMessageLengthFieldSize;
+  nested_message_ = message;
+}
+
+void ProtoZeroMessage::EndNestedMessage() {
+  size_ += nested_message_->Finalize();
+  nested_message_ = nullptr;
+}
+
+}  // namespace v2
+}  // namespace tracing