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

components/tracing/core/proto_zero_message.h

+// 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.
+
+#ifndef COMPONENTS_TRACING_CORE_PROTO_ZERO_MESSAGE_H_
+#define COMPONENTS_TRACING_CORE_PROTO_ZERO_MESSAGE_H_
+
+#include <stdint.h>
+
+#include "base/compiler_specific.h"
+#include "base/gtest_prod_util.h"
+#include "base/logging.h"
+#include "base/macros.h"
+#include "base/template_util.h"
+#include "components/tracing/core/scattered_stream_writer.h"
+#include "components/tracing/tracing_export.h"
+
+namespace tracing {
+namespace v2 {
+
+// Base class extended by the proto C++ stubs generated by the ProtoZero
+// compiler (see //components/tracing/tools/). This class provides the minimal
+// runtime required to support append-only operations and is desiged for
+// performance. None of the methods require any dynamic memory allocation.
+class TRACING_EXPORT ProtoZeroMessage {
+ public:
+  // Commits all the changes to the buffer (backfills the size field of this and
+  // all nested messages) and seals the message. Returns the size of the message
+  // (and all nested sub-messages), without taking into account any chunking.
+  // Finalize is idempotent and can be called several times w/o side effects.
+  size_t Finalize();
+
+  // Optional. If is_valid() the corresponding memory region (which length

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

In what sense is it "Optional"? Is it always present when is_valid() is true?

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

"which length" is very difficult to understand. "with length"?

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

nit: add a comma after "is_valid()"

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

Done.

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

Optional in the sense that you can not call set_size_field (in which case
is_valid() == false) at all.

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

its should make it clearer.

+  // necessarily == proto::kMessageLengthFieldSize) is backfilled with the size
+  // of this message (minus |size_already_written| below) when the message is
+  // finalized. This is the mechanism used by messages to backfill their
+  // corresponding size field in the parent message.
+  ContiguousMemoryRange size_field() const { return size_field_; }
+  void set_size_field(const ContiguousMemoryRange& reserved_range) {
+    size_field_ = reserved_range;
+  }
+
+  // This is to deal with case of backfilling the size of a root (non-nested)
+  // message which is split into multiple chunks. Upon finalization only the
+  // partial size that lies in the last chunk has to be backfilled.
+  void inc_size_already_written(size_t size) { size_already_written_ += size; }
+
+ protected:
+  ProtoZeroMessage();
+
+  // Clears up the state, allowing the message to be reused as a fresh one.
+  void Reset(ScatteredStreamWriter*);
+
+  void AppendVarIntU64(uint32_t field_id, uint64_t value);
+  void AppendVarIntU32(uint32_t field_id, uint32_t value);
+  void AppendFloat(uint32_t field_id, float value);
+  void AppendDouble(uint32_t field_id, double value);
+  void AppendString(uint32_t field_id, const char* str);
+  void AppendBytes(uint32_t field_id, const void* value, size_t size);
+  // TODO(kraynov): implement AppendVarIntS32/64(...) w/ zig-zag encoding.
+
+  // Begins a nested message, using the static storage provided by the parent
+  // class (see comment in |nested_messages_arena_|). The nested message ends
+  // either when Finalize() is called or when any other Append* method is called
+  // in the parent class.
+  // The template argument T is supposed to be a stub class auto generated from
+  // a .proto, hence a subclass of ProtoZeroMessage.
+  template <class T>
+  T* BeginNestedMessage(uint32_t field_id) {
+    T* message = reinterpret_cast<T*>(nested_messages_arena_);

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

How about adding a static assert here that sizeof(T) == sizeof(ProtoZeroMessage)

Also consider using a placement new instead of reinterpret_cast.

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

Oh yes definitely, we really need this to prevent subclasses to introduce extra
state (they are auto generate though, but you never know).
At some point I though to that but then I forgot along the way. Thanks.
So this is interesting, in the beginning I was doing a placement new indeed. The
problem with placement new is that invokes the constructor (which itself is a
good thing). The problem with that is that I found out that uninitialized fields
can be actually initialized (the usual C++ undefined behavior thing).
Specifically what was happening is that in clang debug builds the arena gets
initialized even if it shouldn't. As a result the arena of nested messages
(which by design overflows the parent storage) did clobber the stack/heap
(Depending where the root message was allocated).
This, instead, avoids to invoke any ctor and avoid any unwanted initialization.
I checked this code locally with Asan and it seems happy.

+    BeginNestedMessageInternal(field_id, message);
+    return message;
+  }
+
+  void EndNestedMessage();

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

Maybe add a comment saying that this is called by Finalize and Append* methods?

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

Done. actually realized that this should be private.

+
+ private:
+  friend class ProtoZeroMessageTest;
+  FRIEND_TEST_ALL_PREFIXES(ProtoZeroMessageTest, BasicTypesNoNesting);
+  FRIEND_TEST_ALL_PREFIXES(ProtoZeroMessageTest, BackfillSizeOnFinalization);
+  FRIEND_TEST_ALL_PREFIXES(ProtoZeroMessageTest, NestedMessagesSimple);
+  FRIEND_TEST_ALL_PREFIXES(ProtoZeroMessageTest, StressTest);
+
+  enum : uint32_t { kMaxNestingDepth = 8 };
+
+  void BeginNestedMessageInternal(uint32_t field_id, ProtoZeroMessage*);
+
+  inline void WriteToStream(const uint8_t* src_begin, const uint8_t* src_end) {
+    const size_t size = static_cast<size_t>(src_end - src_begin);
+    stream_writer_->WriteBytes(src_begin, size);
+    size_ += size;
+  }
+
+  // Only PODs fields are allowed. This class' dtor is never called.

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

supernit: From what I could find, "class's" is preferred over "class'" (even
though both are correct).

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

nit: "POD fields" (no need for the double plural)

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

Done.

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

"The Elements of Style and the Canadian Press Stylebook prefer the form in -s's
with the exception of Biblical and classical proper names and common phrases
that do not take the extra s"
I found this class pretty biblical :P (in the sense that took ages to me to
write)

Done.

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

:-D :-D :-D

+  // See the comment on the static_assert in the the corresponding .cc file.
+
+  // The stream writer interface used for the serialization.
+  ScatteredStreamWriter* stream_writer_;
+
+  // Keeps track of the size of the current message.
+  size_t size_;
+
+  ContiguousMemoryRange size_field_;
+  size_t size_already_written_;
+
+  // Used to detect attemps to create messages with a nesting level >
+  // kMaxNestingDepth. |nesting_depth_| == 0 for root (non-nested) messages.
+  uint32_t nesting_depth_;
+
+#if DCHECK_IS_ON()
+  // When true no more changes to the message are allowed. This is to DCHECK

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

nit: add comma after "true"

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

Done.

+  // attempts of writing to a message which has been Finalize()-d.
+  bool sealed_;
+#endif
+
+  // Pointer to the last child message created through BeginNestedMessage(), if
+  // any. nullptr otherwise. There is no need to keep track of more than one
+  // message per nesting level as he proto-zero API contract mandates that

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

s/as he/as the/

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

Done.

+  // nested fields can be filled only in a stacked fashion. In othwe words,

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

s/othwe/other/ (one key to the right :-D)

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

Done.

+  // nested messages are finalized and sealed when any other field is set in the
+  // parent messag (or the parent message itself is finalized) and cannot be

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

nit: s/messag\b/message/

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

Done.

+  // accessed anymore after that point.

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

nit: s/after that point/afterwards/

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

Done.

+  ProtoZeroMessage* nested_message_;
+
+  // The root message owns the storage for all its nested messages, up to a max
+  // of kMaxNestingDepth levels (see the .cc file). Note that the boundaries of
+  // the arena are meaningful only for the root message. The static_assert in
+  // the .cc file guarantees that the sizeof(nested_messages_arena_) is enough
+  // to contain up to kMaxNestingDepth messages.
+  ALIGNAS(sizeof(void*)) uint8_t nested_messages_arena_[512];
+
+  // DO NOT add any fields below nested_messages_arena_. The memory layout of

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

shouldn't this be "|nested_message_arena|"?

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

Done.

+  // nested messaged would overflow the storage allocated by the root message.

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

s/messaged/messages/

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

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(ProtoZeroMessage);
+};
+
+}  // namespace v2
+}  // namespace tracing
+
+#endif  // COMPONENTS_TRACING_CORE_PROTO_ZERO_MESSAGE_H_