Tracing V2: Proto message and utils improvements.

components/tracing/core/proto_utils.h

Index: components/tracing/core/proto_utils.h
diff --git a/components/tracing/core/proto_utils.h b/components/tracing/core/proto_utils.h
index 68599199688e98f0182aeb874a4e6e15e0585f2d..016975d298757b8ccba71af153afd3194c067004 100644
--- a/components/tracing/core/proto_utils.h
+++ b/components/tracing/core/proto_utils.h
@@ -10,13 +10,14 @@
 #include <type_traits>
 
 #include "base/logging.h"
-#include "base/macros.h"
-#include "components/tracing/tracing_export.h"
 
 namespace tracing {
 namespace v2 {
 namespace proto {
 
+// TODO(kraynov): Change namespace to tracing::proto::internal.
+// This is required in headers and it's too low-level to be exposed.

[Primiano Tucci (use gerrit), 2016/08/24 08:33:53]

This was the entire point of "proto" namespace. Not expose internal details that
deal with proto.

[kraynov, 2016/08/24 13:05:43]

Acknowledged.

+
 // See https://developers.google.com/protocol-buffers/docs/encoding wire types.
 
 enum : uint32_t {
@@ -30,69 +31,64 @@ enum : uint32_t {
 constexpr size_t kMessageLengthFieldSize = 4;
 constexpr size_t kMaxMessageLength = (1u << (kMessageLengthFieldSize * 7)) - 1;
 
-// Returns the number of bytes sufficient to encode the largest
-// |int_size_in_bits|-bits integer using a non-redundant varint encoding.
-template <typename T>
-constexpr size_t GetMaxVarIntEncodedSize() {
-  return (sizeof(T) * 8 + 6) / 7;
-}
+// Field tag is encoded as 32-bit varint (5 bytes at most).
+// Largest value of simple (not length-delimited) field is 64-bit varint
+// (10 bytes at most). 15 bytes buffer is enough to store a simple field.
+constexpr size_t kMaxTagEncodedSize = 5;
+constexpr size_t kMaxSimpleFieldEncodedSize = 15;
 
-// Variable-length field types: (s)int32, (s)int64, bool, enum.
-inline uint32_t MakeTagVarInt(uint32_t field_id) {
+// Proto types: (int|uint|sint)(32|64), bool, enum.
+constexpr uint32_t MakeTagVarInt(uint32_t field_id) {
   return (field_id << 3) | kFieldTypeVarInt;
 }
 
-// Length-limited field types: string, bytes, embedded messages.
-inline uint32_t MakeTagLengthDelimited(uint32_t field_id) {
-  return (field_id << 3) | kFieldTypeLengthDelimited;
+// Proto types: fixed64, sfixed64, fixed32, sfixed32, double, float.
+template <typename T>
+constexpr uint32_t MakeTagFixed(uint32_t field_id) {
+  static_assert(sizeof(T) == 8 || sizeof(T) == 4, "Value must be 4 or 8 bytes");
+  return (field_id << 3) |
+         (sizeof(T) == 8 ? kFieldTypeFixed64 : kFieldTypeFixed32);
 }
 
-// 32-bit fixed-length field types: fixed32, sfixed32, float.
-inline uint32_t MakeTagFixed32(uint32_t field_id) {
-  return (field_id << 3) | kFieldTypeFixed32;
+// Proto types: string, bytes, embedded messages.
+constexpr uint32_t MakeTagLengthDelimited(uint32_t field_id) {
+  return (field_id << 3) | kFieldTypeLengthDelimited;
 }
 
-// 64-bit fixed-length field types: fixed64, sfixed64, double.
-inline uint32_t MakeTagFixed64(uint32_t field_id) {
-  return (field_id << 3) | kFieldTypeFixed64;
+// Proto tipes: sint64, sint32.
+template <typename T>
+inline typename std::make_unsigned<T>::type ZigZagEncode(T value) {
+  return (value << 1) ^ (value >> (sizeof(T) * 8 - 1));
 }
 
 template <typename T>
-inline uint8_t* WriteVarIntInternal(T value, uint8_t* target) {
-  static_assert(std::is_unsigned<T>::value, "value must be unsigned");
-  while (value >= 0x80) {
-    *target++ = static_cast<uint8_t>(value | 0x80);
-    value >>= 7;
+inline uint8_t* WriteVarInt(T value, uint8_t* target) {
+  // Avoid arithmetic (sign expanding) shifts.
+  typedef typename std::make_unsigned<T>::type unsigned_T;
+  unsigned_T unsigned_value = static_cast<unsigned_T>(value);
+
+  while (unsigned_value >= 0x80) {
+    *target++ = static_cast<uint8_t>(unsigned_value) | 0x80;
+    unsigned_value >>= 7;
   }
-  *target = static_cast<uint8_t>(value);
+  *target = static_cast<uint8_t>(unsigned_value);
   return target + 1;
 }
 
-inline uint8_t* WriteVarIntU32(uint32_t value, uint8_t* target) {
-  return WriteVarIntInternal<uint32_t>(value, target);
-}
-
-inline uint8_t* WriteVarIntU64(uint64_t value, uint8_t* target) {
-  return WriteVarIntInternal<uint64_t>(value, target);
-}
-
-// TODO(kraynov): add support for signed integers and zig-zag encoding.
-
 // Writes a fixed-size redundant encoding of the given |value|. This is
-// used to backfill fixed-size reservations for the length field using a
+// used to backfill fixed-size reservations for the length of field using a

[Primiano Tucci (use gerrit), 2016/08/24 08:33:53]

no, the meaning here was really "the lenght field", that is the field that tells
the length.

[kraynov, 2016/08/24 13:05:43]

Let's be consistent with protobuf terminology:
- Field means some data corresponding to a number in message descriptor
- Tag means field type and number tuple
- "Length" is a part of length-delimited field

 // non-canonical varint encoding (e.g. \x81\x80\x80\x00 instead of \x01).
 // See https://github.com/google/protobuf/issues/1530.
 // In particular, this is used for nested messages. The size of a nested message
-// is not known until all its field have been written. A fixed amount of bytes
-// is reserved to encode the size field and backfilled at the end.
-template <size_t LENGTH>
-void WriteRedundantVarIntU32(uint32_t value, uint8_t* buf) {
-  for (size_t i = 0; i < LENGTH; ++i) {
-    const uint8_t msb = (i < LENGTH - 1) ? 0x80 : 0;
-    buf[i] = static_cast<uint8_t>((value & 0x7F) | msb);
+// is not known until all its field have been written. |kMessageLengthFieldSize|
+// bytes are reserved to encode the size field and backfilled at the end.
+inline void WriteRedundantLength(uint32_t value, uint8_t* buf) {

[Primiano Tucci (use gerrit), 2016/08/24 08:33:53]

WriteRedundantLength is IMHO a bad name.
What is a redundant length?
The thing that is redundant here is the varint encoding. You just removed the
most important part of the name, that is "VarInt".

[kraynov, 2016/08/24 13:05:43]

The only purpose of this method is overwriting placeholder for length. It takes
exactly |kMessageLengthFieldSize| (not the best name for constant because of
collision with "field" meaning in protobuf, but it's better than something
non-obvious like |kMessageLengthSize| or verbose
|kMessageLengthPlaceholderSize|) number of bytes. It basically writes the length
value for field, VarInt is not a point in this abstraction.

+  DCHECK_LE(value, kMaxMessageLength) << "Message is too long";
+  for (size_t i = 0; i < kMessageLengthFieldSize; ++i) {
+    const uint8_t msb = (i < kMessageLengthFieldSize - 1) ? 0x80 : 0;
+    buf[i] = static_cast<uint8_t>(value) | msb;
     value >>= 7;
   }
-  DCHECK_EQ(0u, value) << "Buffer too short to encode the given value";
 }
 
 }  // namespace proto