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

components/tracing/core/proto_zero_message.cc

Index: components/tracing/core/proto_zero_message.cc
diff --git a/components/tracing/core/proto_zero_message.cc b/components/tracing/core/proto_zero_message.cc
new file mode 100644
index 0000000000000000000000000000000000000000..188b73ed6a02a9449c99d4f31f707186ad8643aa
--- /dev/null
+++ b/components/tracing/core/proto_zero_message.cc
@@ -0,0 +1,164 @@
+// 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 "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 NOT add any code here, use the Reset() method below instead.
+  // Ctor and Dtor of ProtoZeroMessage are never called, with the exeception
+  // of root (non-nested) messages. Nested messages are allocated in the
+  // |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,

[oystein (OOO til 10th of July), 2016/07/12 21:03:29]

Why are these static_asserts in the constructor? Shouldn't they be in the class
declaration instead? It'd make it a little clearer that the constructor is (and
should be) empty.

[Primiano Tucci (use gerrit), 2016/07/13 10:25:07]

Two things here:
1) If I add them to the .h that will make compilation slower for all translation
units that pull that header (possibly a lot)
2)  the cocnret problem is that the static_assert args are still subjected to
public/private protection rules, it's a bit weird. If I just put it at the end
of the header I get an error "nested_message_arena_ is a private field yada
yada". So I have to put this code inside a context which is allowed to access
private fields, even if this is just a compile time check.

+                "ProtoZeroMessage must be trivially destructible");
+
+  static_assert(
+      sizeof(ProtoZeroMessage::nested_messages_arena_) >=
+          kMaxNestingDepth * (sizeof(ProtoZeroMessage) -
+                              sizeof(ProtoZeroMessage::nested_messages_arena_)),
+      "ProtoZeroMessage::nested_messages_arena_ is too small");
+}
+
+// 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;
+#endif
+}
+
+void ProtoZeroMessage::AppendVarIntU64(uint32_t field_id, uint64_t value) {
+  if (nested_message_)
+    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 .
+  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);

[oystein (OOO til 10th of July), 2016/07/12 21:03:29]

Feels like this should be a stronger check (CHECK_LT?). Or is the idea that it's
the stream writer's responsibility to check for buffer overruns?

[Primiano Tucci (use gerrit), 2016/07/13 10:25:07]

this cannot overrun and cause memory corruption. Even if you pass a gigantic
buffer here, the stream_writer_ will split the write into chunks and keep
requesting chunks until the buffer has been written (or the delegate returns a
nullptr, in which case *boom*).
The only reason of this dcheck is that if you manage to write a gigantic buffer,
the size of the message will be truncated to 4 bytes and the importer will fail.
Given that it is unlikely and doesn't cause a memory corruption, doesn't feel it
justifies a CHECK.
Having said this I am removing this DCHECK here at all, thinking more this
dcheck is just misplaced here, for the following reason:
- There is no problem here writing a large buffer, as this size is actually
encoded as a conventional (i.e. non-fixed-size) varint.
- The thing that will be truncated is the size field of the entire message (that
one is a fixed-length varint)
- But the size field of the message can overflow even if a single write doesn't
(imagine you make 10000 calls to AppendBytes with a small size).
The place there this check should be in place is ::Finalize(). Here is pointless
and confusing. removed.

+  uint8_t data[2 * proto::GetMaxVarIntEncodedSize<uint32_t>()];
+  uint8_t* data_end =
+      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(!sealed_);

[oystein (OOO til 10th of July), 2016/07/12 21:03:29]

DCHECK_IS_ON() (at least that's used around the DCHECK(sealed_) in the header, I
assume that's because the variable is always referenced?)

[Primiano Tucci (use gerrit), 2016/07/13 10:25:07]

Oops yes. forgot this at last minute. I suppose this is what made most bots red
:)

+    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 =
+      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