f
diff --git a/include/perfetto/protozero/message.h b/include/perfetto/protozero/message.h index ef360ca..2ec83c6 100644 --- a/include/perfetto/protozero/message.h +++ b/include/perfetto/protozero/message.h
@@ -41,17 +41,17 @@ class MessageArena; class MessageHandleBase; -// Encoding used for nested messages. The caller picks it when resetting the -// root message and all its children use the same one. +// Encoding for nested messages. The caller selects it in the root's Reset() +// call. Each child inherits its parent's encoding. // -// kLengthDelimited is normal protobuf. It reserves space for the message -// length and fills it in when the message is finalized. +// kLengthDelimited uses standard protobuf. It reserves a length field before +// each nested message. Finalize() writes the message's length into that field. // // kProtoGroup is the append-only encoding used by tracing v2. It opens a nested // message with a protobuf start-group tag and closes it with // proto_utils::kProtoGroupEndByte. Unlike a protobuf group, the closing byte -// does not repeat the field id. ProtoRewriter converts an assembled packet back -// into length-delimited protobuf. +// does not repeat the field id. ProtoRewriter converts the reassembled packet +// to length-delimited protobuf. enum class NestedMessageEncoding : uint8_t { kLengthDelimited = 0, kProtoGroup = 1, @@ -74,38 +74,39 @@ // static_assert()s in the Reset() method. Message() = default; - // Clears up the state, allowing the message to be reused as a fresh one. - // Uses length-delimited encoding and initializes a root message. + // Resets this object as an empty root message with length-delimited encoding. void Reset(ScatteredStreamWriter*, MessageArena*); - // As above, but selects the encoding and whether this is a root message. - // In proto-group mode only nested messages append a closing byte. + // Resets this object with the selected encoding and root status. + // In proto group mode, Finalize() appends a closing byte only if |is_root| + // is false. The packet boundary marks the end of a root message. void Reset(ScatteredStreamWriter*, MessageArena*, NestedMessageEncoding, bool is_root); - // Commits all the changes to the buffer and seals the message. In - // length-delimited mode this backfills the size field of this and all nested - // messages. In proto-group mode each nested message appends its close byte - // and the root appends nothing. 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. - // Short length-delimited messages may be compacted in memory into the size - // field, since their size can be represented with fewer than - // proto_utils::kMessageLengthFieldSize bytes. + // Finalizes all open children, then seals this message: + // - Length-delimited: writes the encoded size into the reserved length field, + // if present. Short messages can use fewer than + // proto_utils::kMessageLengthFieldSize bytes for that field through + // compaction within the current chunk. + // - Proto group: appends a closing byte for a nested message. The root has no + // closing byte. + // + // Returns the encoded size, including nested messages and any closing byte. + // The size excludes this message's tag and reserved length field. + // Repeated calls return the same size without further writes. uint32_t Finalize(); - // Optional. If is_valid() == true, the corresponding memory region (its - // length == proto_utils::kMessageLengthFieldSize) is backfilled with the size - // of this message. This is the mechanism used by messages to backfill their - // corresponding size field in the parent message. In most cases this is only - // used for nested messages and the ScatteredStreamWriter::Delegate (e.g. - // TraceWriterImpl), takes case of the outer message. + // Optional reserved length field with proto_utils::kMessageLengthFieldSize + // bytes. If non-null, Finalize() writes this message's encoded size there + // and can compact the field as described above. + // TraceWriterImpl tracks root fragment sizes separately, so its root has no + // length field. In proto group mode, this pointer must always be null. uint8_t* size_field() const { return size_field_; } void set_size_field(uint8_t* size_field) { size_field_ = size_field; } - // Returns the encoding a new child should inherit. + // Returns the encoding each new child inherits. NestedMessageEncoding nested_message_encoding() const { return encoding_; } Message* nested_message() { return nested_message_; } @@ -212,7 +213,7 @@ // proto-encoded and each field has a proto preamble. // // In kProtoGroup mode, |data| must not contain protobuf group fields. Wire - // types 3 and 4 belong to the surrounding proto-group framing in that mode. + // types 3 and 4 belong to the surrounding proto group framing in that mode. void AppendRawProtoBytes(const void* data, size_t size) { if (nested_message_) EndNestedMessage(); @@ -260,7 +261,7 @@ // has kMessageLengthFieldSize bytes and may be unaligned. Finalize() writes // this message's encoded size there, then sets this pointer to null. // - // In proto-group mode, this pointer is unused and always null. That encoding + // In proto group mode, this pointer is unused and always null. That encoding // reserves no length field. A nested message appends a closing byte when it // finishes, and the packet boundary marks the end of the root message. uint8_t* size_field_; @@ -285,13 +286,16 @@ // Selected by the root and inherited by every nested message. NestedMessageEncoding encoding_; - // Whether this message is the packet root. In proto-group mode, Finalize() + // Whether this message is the packet root. In proto group mode, Finalize() // closes nested messages with an end byte. The packet boundary closes the // root, which has no start tag or end byte. // - // MessageHandleBase finalizes through Message*, and packet handles expose - // the generated message type. Both call Message::Finalize(), so the root - // identity must be stored in Message even when RootMessage owns the arena. + // Finalization does not always use the RootMessage type: + // - MessageHandleBase calls Finalize() through Message*. + // - Packet handles expose the generated message type, which inherits + // Message::Finalize(). + // Both paths must recognize the root. An override in RootMessage alone + // cannot handle them, so Message stores the root identity. // // Example: a root packet containing just one nested message: //
diff --git a/include/perfetto/protozero/proto_utils.h b/include/perfetto/protozero/proto_utils.h index 9b8805f..0804cdb 100644 --- a/include/perfetto/protozero/proto_utils.h +++ b/include/perfetto/protozero/proto_utils.h
@@ -48,7 +48,7 @@ kFixed32 = 5, }; -// Tracing v2's private proto-group format borrows the protobuf start-group +// Tracing v2's private proto group format borrows the protobuf start-group // wire type (3). It closes each nested message with a single 0x04 byte instead // of a standard end-group tag that repeats the opening field number. See // kProtoGroupEndByte below for the framing and RFC 0014 for the design: @@ -66,9 +66,10 @@ // close current nested: 0x04 // root: no wrapper and no close byte // -// Opens carry the field number. 0x04 is field-zero/end-group, which cannot be -// an ordinary field, and closes the innermost message. Packet framing bounds -// the root, so it needs neither marker. +// The opening tag carries the field number. The closing byte, 0x04, encodes +// field number zero with the end-group wire type. Field zero is invalid in +// ordinary protobuf, so this byte closes the innermost message at a field +// boundary. The packet boundary marks the end of the root. constexpr uint8_t kProtoGroupEndByte = 0x04; // This is the type defined in the proto for each field. This information @@ -223,7 +224,7 @@ static_cast<uint32_t>(ProtoWireType::kLengthDelimited); } -// Opens a nested message in the proto-group encoding described above. +// Opens a nested message in the proto group encoding described above. constexpr uint32_t MakeTagStartGroup(uint32_t field_id) { return (field_id << kFieldTypeNumBits) | kWireTypeStartGroup; }
diff --git a/include/perfetto/public/abi/data_source_abi.h b/include/perfetto/public/abi/data_source_abi.h index 3df8c87..4d51834 100644 --- a/include/perfetto/public/abi/data_source_abi.h +++ b/include/perfetto/public/abi/data_source_abi.h
@@ -377,15 +377,16 @@ // used to write data to the packet. The caller must use // PerfettoDsTracerImplPacketEnd() when done. // -// Only supports length-delimited writers. Aborts for ProtoGroup writers +// Only supports length-delimited writers. Aborts for proto group writers // selected by DataSourceConfig.use_tracing_v2. // Use PerfettoDsTracerImplPacketBeginWithEncoding() for new callers. PERFETTO_SDK_EXPORT struct PerfettoStreamWriter PerfettoDsTracerImplPacketBegin( struct PerfettoDsTracerImpl* tracer); -// Encoding the nested messages of a packet must use. It comes back together -// with the packet's stream writer and PerfettoDsRootTracePacketBegin() passes -// it on when it initializes the root message. +// Encoding required by the packet's stream writer. +// PerfettoDsTracerImplPacketBeginWithEncoding() returns both values together. +// PerfettoDsRootTracePacketBegin() initializes the root with this encoding, +// which each nested message inherits. enum PerfettoDsPacketEncoding { PERFETTO_DS_PACKET_ENCODING_LENGTH_DELIMITED = 0, PERFETTO_DS_PACKET_ENCODING_PROTO_GROUP = 1, @@ -396,8 +397,9 @@ uint32_t encoding; // PerfettoDsPacketEncoding. }; -// Starts a packet and returns its writer and encoding. Call -// PerfettoDsTracerImplPacketEnd() when done. +// Starts a packet and returns its writer and required encoding. +// The caller must use that encoding for all nested messages in the packet. +// Call PerfettoDsTracerImplPacketEnd() after the packet is complete. PERFETTO_SDK_EXPORT struct PerfettoDsPacketBeginResult PerfettoDsTracerImplPacketBeginWithEncoding( struct PerfettoDsTracerImpl* tracer);
diff --git a/include/perfetto/public/data_source.h b/include/perfetto/public/data_source.h index f2b89cf..95b0172 100644 --- a/include/perfetto/public/data_source.h +++ b/include/perfetto/public/data_source.h
@@ -277,9 +277,9 @@ struct perfetto_protos_TracePacket msg; }; -// The data-source ABI and pb_msg.h define separate encoding enums. Convert with -// a switch rather than a cast, so an unknown value aborts below instead of -// picking the wrong framing. +// Converts the data-source ABI encoding to pb_msg.h's separate enum. +// Unknown values abort because the caller cannot encode a packet without a +// supported format. static inline enum PerfettoPbMsgEncoding PerfettoDsPacketEncodingToPbMsg( uint32_t encoding) { switch (encoding) { @@ -289,8 +289,8 @@ return PERFETTO_PB_MSG_ENCODING_PROTO_GROUP; } - // The caller and tracing library disagree about the encoding ABI. Choosing - // another framing would corrupt the packet. + // The caller does not support the encoding returned by the tracing library. + // Substituting another encoding would corrupt the packet. abort(); }
diff --git a/include/perfetto/public/pb_macros.h b/include/perfetto/public/pb_macros.h index 5565e5a..931fd59 100644 --- a/include/perfetto/public/pb_macros.h +++ b/include/perfetto/public/pb_macros.h
@@ -25,7 +25,8 @@ // This header contains macros that define types and accessors for protobuf // messages. // -// ProtoGroup supports scalar, complete-value and nested-message accessors. +// The proto group encoding supports scalar, complete-value and nested-message +// accessors. // Incremental STRING/PACKED begin/append/end accessors abort. // See PerfettoPbMsgEncoding in pb_msg.h. //
diff --git a/include/perfetto/public/pb_msg.h b/include/perfetto/public/pb_msg.h index f4e90ec..c1f77ff 100644 --- a/include/perfetto/public/pb_msg.h +++ b/include/perfetto/public/pb_msg.h
@@ -33,13 +33,13 @@ // proto_utils.h. #define PROTOZERO_MESSAGE_LENGTH_FIELD_SIZE 4 -// Encoding used for nested messages. The root picks it when it is initialized -// and all its children use the same one. +// Encoding for nested messages. The caller selects it when it initializes the +// root. Each child inherits its parent's encoding. enum PerfettoPbMsgEncoding { - // Normal protobuf. The nested-message length is filled in on finalization. + // Standard protobuf. Finalization fills the reserved nested-message length. PERFETTO_PB_MSG_ENCODING_LENGTH_DELIMITED = 0, - // Append-only proto-group encoding used by tracing v2. + // Append-only proto group encoding used by tracing v2. // - Supports scalars, whole string/bytes/packed values and nested messages. // - Incremental STRING/PACKED builders abort before writing a start tag. // See PERFETTO_PB_PROTO_GROUP_END_BYTE in pb_utils.h for the wire format. @@ -56,9 +56,9 @@ // unaligned and has PROTOZERO_MESSAGE_LENGTH_FIELD_SIZE bytes. // PerfettoPbMsgFinalize() fills in the length, then clears this pointer. // - // In proto-group mode, a nested message ends with a closing byte and the - // root ends at the packet boundary. Neither reserves a length before its - // contents, so this pointer stays NULL. + // In proto group mode, this pointer is unused and always NULL. This encoding + // reserves no length field. A nested message appends a closing byte, and the + // packet boundary marks the end of the root. uint8_t* size_field; // Current size of the buffer. @@ -68,37 +68,38 @@ struct PerfettoPbMsg* nested; - // NULL for the root; otherwise the message that opened this child. - // In proto-group mode, PerfettoPbMsgFinalize() uses this to distinguish: - // - Nested messages: append a closing byte after finalizing their children. - // - Root messages: finalize their children without appending a closing byte - // for the root itself. The caller ends the packet, whose boundary marks - // the end of the root message. + // NULL for the root. Otherwise, points to the message that opened this child. + // In proto group mode, PerfettoPbMsgFinalize() uses this to distinguish: + // - Nested messages: finalize their children, then append a closing byte. + // - Root messages: finalize their children without a closing byte for the + // root. The caller ends the packet, whose boundary marks the root's end. struct PerfettoPbMsg* parent; // Selected by the root and inherited by its children. enum PerfettoPbMsgEncoding encoding; - // A caller can call PerfettoPbMsgFinalize(child), then close that child - // with PerfettoPbMsgEndNested(parent), which calls Finalize(child) again. + // A child can be finalized twice through this sequence: + // 1. The caller calls PerfettoPbMsgFinalize(child) directly. + // 2. The caller calls PerfettoPbMsgEndNested(parent), which calls + // PerfettoPbMsgFinalize(child) again. + // // - Length-delimited: the first call writes the length and clears - // size_field. The second call has nothing left to patch and is safe. - // - Proto-group: the first call appends a closing byte. A second closing - // byte would corrupt the packet. There is no size_field to clear, so - // this flag tells the second call to skip writing that byte again. + // size_field. The second call has no length field to patch. + // - Proto group: size_field is always NULL. The first call appends a closing + // byte and sets this flag. The second call must not append another closing + // byte because it would close the parent or corrupt the packet. bool proto_group_end_written; }; -// Initializes a root message. See PerfettoPbMsgEncoding for supported -// operations. +// Initializes a root message with the selected encoding. Unknown encodings +// abort. See PerfettoPbMsgEncoding for supported operations. static inline void PerfettoPbMsgInitWithEncoding( struct PerfettoPbMsg* msg, struct PerfettoPbMsgWriter* writer, enum PerfettoPbMsgEncoding encoding) { if (encoding != PERFETTO_PB_MSG_ENCODING_LENGTH_DELIMITED && encoding != PERFETTO_PB_MSG_ENCODING_PROTO_GROUP) { - // Choosing another framing for an unknown encoding would corrupt the - // packet. + // A substitute encoding would produce a different wire format. abort(); } msg->size_field = PERFETTO_NULL; @@ -110,7 +111,7 @@ msg->proto_group_end_written = false; } -// Existing callers continue to write normal length-delimited protobuf. +// Initializes a root message with the default length-delimited encoding. static inline void PerfettoPbMsgInit(struct PerfettoPbMsg* msg, struct PerfettoPbMsgWriter* writer) { PerfettoPbMsgInitWithEncoding(msg, writer, @@ -128,7 +129,7 @@ static inline void PerfettoPbMsgPatchStack(struct PerfettoPbMsg* msg) { uint8_t* const cur_range_end = msg->writer->writer.end; uint8_t* const cur_range_begin = msg->writer->writer.begin; - // Stop at the root or a proto-group message; neither has a length field. + // Stop at the root or a proto group message. Neither has a length field. while (msg && msg->size_field && cur_range_begin <= msg->size_field && msg->size_field < cur_range_end) { PerfettoPbMsgPatch(msg); @@ -284,12 +285,14 @@ parent->nested = nested; } -// STRING and PACKED fields use the begin/end API too, but their payload is a -// length-delimited value rather than a nested message. -// - Length-delimited: reserve the length and backfill it when the field closes. -// - Proto-group: abort before writing the tag. PerfettoPbMsgAppendBytes() may -// publish the payload before its length is known, so a reserved length could -// not be patched later. +// STRING and PACKED fields require a length before their payload in both +// encodings. Their incremental begin/append/end accessors handle it as follows: +// - Length-delimited: reserve the length and fill it when the field closes. +// - Proto group: abort before the tag is written. PerfettoPbMsgAppendBytes() +// can release a fragment before the field closes. The reader can copy that +// fragment while the total length is still unknown. Append-only writes +// cannot update a length field in those published bytes. +// // TODO(sashwinbalaji): Buffer the value and emit it when the field closes if a // tracing v2 caller needs incremental fields. static inline void PerfettoPbMsgBeginLengthDelimitedField( @@ -316,8 +319,8 @@ if (PERFETTO_UNLIKELY(msg->encoding == PERFETTO_PB_MSG_ENCODING_PROTO_GROUP)) { - // A child writes its end byte once. The root only finalizes its children; - // its packet boundary takes the place of an end byte (see |parent| above). + // A child appends its closing byte once. The root only finalizes its + // children because the packet boundary marks its end. See `parent` above. if (msg->parent && !msg->proto_group_end_written) { PerfettoPbMsgAppendByte( msg, PERFETTO_STATIC_CAST(uint8_t, PERFETTO_PB_PROTO_GROUP_END_BYTE));
diff --git a/include/perfetto/public/pb_utils.h b/include/perfetto/public/pb_utils.h index 17d62cc..075423e 100644 --- a/include/perfetto/public/pb_utils.h +++ b/include/perfetto/public/pb_utils.h
@@ -42,15 +42,19 @@ PERFETTO_STATIC_CAST(uint32_t, wire_type); } -// Constants for the append-only nested-message encoding used by tracing v2: +// Tracing v2's private proto group encoding uses wire type 3 to open a nested +// message. The closing byte does not repeat the field number, so this differs +// from standard protobuf groups. See RFC 0014: +// https://github.com/google/perfetto/discussions/4508. // // open nested field f: varint((f << 3) | 3) // close current nested: 0x04 // root: no wrapper and no close byte // -// Opens carry the field number. 0x04 is field-zero/end-group, which cannot be -// an ordinary field, and closes the innermost message. Packet framing bounds -// the root, so it needs neither marker. +// The opening tag carries the field number. The closing byte, 0x04, encodes +// field number zero with the end-group wire type. Field zero is invalid in +// ordinary protobuf, so this byte closes the innermost message at a field +// boundary. The packet boundary marks the end of the root. enum { PERFETTO_PB_PROTO_GROUP_START_WIRE_TYPE = 3, PERFETTO_PB_PROTO_GROUP_END_BYTE = 0x04,
diff --git a/src/protozero/message.cc b/src/protozero/message.cc index 9c85b5e..b0b13f4 100644 --- a/src/protozero/message.cc +++ b/src/protozero/message.cc
@@ -154,17 +154,16 @@ if (encoding_ == NestedMessageEncoding::kProtoGroup) { PERFETTO_DCHECK(!size_field_); - // Packet framing closes the root. Only a nested group needs a close byte. + // The packet boundary marks the end of the root. Only a nested message + // appends kProtoGroupEndByte, after all its children close. if (!is_root_) { const uint8_t end = proto_utils::kProtoGroupEndByte; WriteToStream(&end, &end + 1); } message_state_ = MessageState::kFinalized; } else if (size_field_) { - // Write the length of the nested message a posteriori, using a leading-zero - // redundant varint encoding. This can be nullptr for the root message, - // among many reasons, because the TraceWriterImpl delegate is keeping track - // of the root fragment size independently. + // Write the length into the reserved field. Use all four bytes unless + // the message fits the compaction case below. PERFETTO_DCHECK(!is_finalized()); PERFETTO_DCHECK(size_ < proto_utils::kMaxMessageLength); // @@ -237,9 +236,8 @@ message->Reset(stream_writer_, arena_, encoding_, /*is_root=*/false); if (encoding_ == NestedMessageEncoding::kLengthDelimited) { - // 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. + // The child's length is unknown until Finalize(). Reserve its length field + // now so Finalize() can fill it after the child is complete. message->set_size_field( stream_writer_->ReserveBytes(proto_utils::kMessageLengthFieldSize)); size_ += proto_utils::kMessageLengthFieldSize;
diff --git a/src/protozero/message_unittest.cc b/src/protozero/message_unittest.cc index f88e645..0f63514 100644 --- a/src/protozero/message_unittest.cc +++ b/src/protozero/message_unittest.cc
@@ -96,7 +96,7 @@ return msg; } - // A root using the tracing v2 proto-group encoding. + // Creates a root with tracing v2's proto group encoding. FakeRootMessage* NewProtoGroupMessage() { FakeRootMessage* msg = NewMessage(); msg->Reset(stream_writer_.get(), NestedMessageEncoding::kProtoGroup); @@ -482,7 +482,7 @@ // --------------------------------------------------------------------------- // Nested-message encodings. // -// The proto-group encoding is specified at proto_utils::kProtoGroupEndByte. +// The proto group encoding is specified at proto_utils::kProtoGroupEndByte. // Check exact bytes because this is a wire-format contract. // --------------------------------------------------------------------------- @@ -496,9 +496,9 @@ NestedMessageEncoding::kLengthDelimited); msg->Finalize(); - // Field 1 as wire type 2 and its length. The empty message fits in one chunk - // and is short, so the canonical path compacts the four reserved bytes down - // to one. The proto-group path must not change this behavior. + // Field 1 uses wire type 2. Its empty message fits in one chunk. Finalize() + // therefore compacts the four reserved length bytes to one. The default must + // retain this behavior after the addition of proto group support. EXPECT_EQ(2u, GetNumSerializedBytes()); EXPECT_EQ("0A00", GetNextSerializedBytes(2)); } @@ -508,7 +508,7 @@ msg->BeginNestedMessage<FakeChildMessage>(1); msg->Finalize(); - // The worked example: 0b 04. + // 0b opens field 1. 04 closes that empty nested message. EXPECT_EQ(2u, GetNumSerializedBytes()); EXPECT_EQ("0B04", GetNextSerializedBytes(2)); } @@ -523,7 +523,7 @@ EXPECT_EQ(grandchild->nested_message_encoding(), NestedMessageEncoding::kProtoGroup); - // Proto-group messages never reserve a length. + // Messages in proto group mode never reserve a length. EXPECT_EQ(nullptr, child->size_field()); EXPECT_EQ(nullptr, grandchild->size_field()); msg->Finalize(); @@ -573,7 +573,7 @@ msg->AppendVarInt(1, 42); msg->Finalize(); - // Just the field: the root's framing belongs to whoever owns the stream. + // Only the scalar field is emitted. The packet boundary closes the root. EXPECT_EQ(2u, GetNumSerializedBytes()); EXPECT_EQ("082A", GetNextSerializedBytes(2)); } @@ -640,8 +640,8 @@ } TEST_F(MessageTest, ProtoGroupFramingCrossesChunkBoundaries) { - // The fixture hands out 16-byte chunks, so a payload this size puts the start - // tag and the end byte in different chunks. + // The fixture supplies 16-byte chunks. The start tag and closing byte must + // appear in different chunks because the nested payload exceeds that size. FakeRootMessage* msg = NewProtoGroupMessage(); Message* child = msg->BeginNestedMessage<FakeChildMessage>(1); for (uint32_t i = 0; i < 8; ++i)
diff --git a/src/shared_lib/data_source.cc b/src/shared_lib/data_source.cc index 4628f9d..2603f0c 100644 --- a/src/shared_lib/data_source.cc +++ b/src/shared_lib/data_source.cc
@@ -609,7 +609,7 @@ struct PerfettoStreamWriter PerfettoDsTracerImplPacketBegin( struct PerfettoDsTracerImpl* tracer) { const auto result = PerfettoDsTracerImplPacketBeginWithEncoding(tracer); - // The legacy entry point cannot tell its caller to use proto-group encoding. + // The legacy entry point cannot tell its caller to use proto group encoding. PERFETTO_CHECK(result.encoding == PERFETTO_DS_PACKET_ENCODING_LENGTH_DELIMITED); return result.writer;
diff --git a/src/shared_lib/pb_msg_unittest.cc b/src/shared_lib/pb_msg_unittest.cc index 652286d..d5c6d99 100644 --- a/src/shared_lib/pb_msg_unittest.cc +++ b/src/shared_lib/pb_msg_unittest.cc
@@ -43,9 +43,8 @@ // Test plumbing. // --------------------------------------------------------------------------- -// A flat, oversized buffer. Nothing written through it comes close to the end, -// so the stream writer's slow paths are never taken; ChunkedCBuffer below is -// what exercises those. +// One buffer large enough for each test message. These tests do not request a +// second range. ChunkedCBuffer below exercises the stream writer's slow paths. class FlatCBuffer { public: FlatCBuffer() { @@ -67,9 +66,9 @@ PerfettoPbMsgWriter writer_{}; }; -// A real protozero::ScatteredStreamWriter behind the C stream writer, handing -// out deliberately small ranges so that everything the C code writes crosses -// several of them. This is what makes PerfettoPbMsgPatchStack() run. +// Uses protozero::ScatteredStreamWriter to supply small ranges to the C writer. +// Messages cross range boundaries, which makes the C runtime call +// PerfettoPbMsgPatchStack() before it requests another range. class ChunkedCBuffer : public protozero::ScatteredStreamWriter::Delegate { public: explicit ChunkedCBuffer(size_t range_size) @@ -89,7 +88,7 @@ PerfettoPbMsgWriter* writer() { return &writer_; } size_t num_ranges() const { return ranges_.size(); } - // Everything written so far, stitched back into one buffer. + // Returns all written bytes in one contiguous buffer. std::vector<uint8_t> bytes() { stream_writer_.set_write_ptr(writer_.writer.write_ptr); std::vector<uint8_t> out; @@ -139,10 +138,11 @@ return std::vector<uint8_t>(buffer.storage, buffer.stream_writer.write_ptr()); } -// The same shape through the C runtime. The call sequences are not identical -// because the two APIs differ in an unrelated way - the C++ runtime closes an -// open nested message when the parent's next field is written, while the C one -// wants an explicit EndNested - but the bytes they produce must match exactly. +// Writes the same message through the C runtime. The two APIs close children +// differently: +// - C++ closes an open child when the caller writes the parent's next field. +// - C requires an explicit PerfettoPbMsgEndNested() call. +// The resulting bytes must match despite these different call sequences. std::vector<uint8_t> BuildWithC(enum PerfettoPbMsgEncoding encoding) { FlatCBuffer buffer; PerfettoPbMsg root{}; @@ -282,8 +282,8 @@ EXPECT_EQ(PerfettoPbMsgFinalize(&root), 7u); EXPECT_EQ(root.size, 7u); - // Field 2, wire type 2; the redundant four-byte length of 2; then the two - // payload bytes. + // Field 2 with wire type 2, followed by the four-byte encoding of length 2 + // and the two payload bytes. const uint8_t kExpected[] = {0x12, 0x82, 0x80, 0x80, 0x00, 0x08, 0x07}; EXPECT_THAT(buffer.bytes(), ElementsAreArray(kExpected)); } @@ -328,8 +328,7 @@ EXPECT_EQ(BuildWithC(PERFETTO_PB_MSG_ENCODING_PROTO_GROUP), BuildWithCpp(protozero::NestedMessageEncoding::kProtoGroup)); - // The exact expected bytes, so a shared misunderstanding cannot make the - // comparison above pass: + // Check explicit bytes as well, since both runtimes can have the same bug: // 08 07 field 1 = 7 // 13 start field 2 // 18 08 field 3 = 8 @@ -344,10 +343,11 @@ } // --------------------------------------------------------------------------- -// STRING and PACKED fields. Their payload is arbitrary bytes, so a proto group -// cannot frame them. A complete setter emits an ordinary length-delimited -// field in either encoding; the piecewise builders need a length that is not -// known yet, so proto-group mode rejects them. +// STRING and PACKED fields contain arbitrary bytes, so group tags cannot frame +// them. They require a length before their payload. +// Complete setters know that length and emit a length-delimited field in either +// encoding. Incremental builders know the length only when the field closes, +// so the append-only proto group encoding rejects them. // --------------------------------------------------------------------------- TEST(PbMsgTest, ProtoGroupWholeValueSettersStayLengthDelimited) { @@ -367,9 +367,9 @@ EXPECT_THAT(buffer.bytes(), ElementsAreArray(kExpected)); } -// The piecewise builders reserve a length and fill it in on end, which the -// append-only encoding cannot do. They must fail at begin, before a start tag -// or any payload byte reaches the packet. +// Incremental builders reserve a length field and fill it at the end. +// Append-only writes cannot update that length. Reject the begin call before +// a tag or payload byte reaches the packet. TEST(PbMsgTest, ProtoGroupRejectsIncrementalString) { FlatCBuffer buffer; PbMsgTestFields root{}; @@ -425,8 +425,8 @@ // --------------------------------------------------------------------------- TEST(PbMsgTest, ProtoGroupMessagesCrossRanges) { - // Small ranges force the stream writer through its slow path. Proto-group - // messages have no size field, so there is nothing to patch on rollover. + // Small ranges force the stream writer through its slow path. Messages in + // proto group mode have no size field, so range changes require no patches. ChunkedCBuffer buffer(/*range_size=*/32); PerfettoPbMsg root{}; InitProtoGroupRoot(&root, &buffer); @@ -438,8 +438,8 @@ PerfettoPbMsgAppendType2Field(&nested, 2, payload.data(), payload.size()); PerfettoPbMsgFinalize(&root); - // If a future change made the ranges big enough to hold everything, this test - // would silently stop testing the slow path. + // Require a range change so the test continues to exercise the slow path + // even if the fixture's buffer sizes change. EXPECT_GT(buffer.num_ranges(), 1u); const std::vector<uint8_t> bytes = buffer.bytes(); @@ -474,8 +474,7 @@ } // --------------------------------------------------------------------------- -// An encoding this build has never heard of must fail before any byte is -// written, not quietly become length-delimited. +// Reject an unknown encoding before any byte reaches the stream. // --------------------------------------------------------------------------- TEST(PbMsgTest, UnknownEncodingAborts) {
diff --git a/src/tracing/v2/BUILD.gn b/src/tracing/v2/BUILD.gn index e8350ad..19270c2 100644 --- a/src/tracing/v2/BUILD.gn +++ b/src/tracing/v2/BUILD.gn
@@ -16,8 +16,8 @@ import("../../../gn/test.gni") # The tracing v2 producer-local shared ring buffer: its ABI, writer and reader. -# TraceWriterV2Impl and ProtoRewriter handle the ProtoGroup fragment payloads. -# Protocol and alternatives: RFC 0046, +# TraceWriterV2Impl and ProtoRewriter handle the proto group fragment payloads. +# Protocol: RFC 0046, # https://github.com/google/perfetto/discussions/7120. # Parent design: RFC 0014, https://github.com/google/perfetto/discussions/4508. source_set("v2") {
diff --git a/src/tracing/v2/proto_rewriter.cc b/src/tracing/v2/proto_rewriter.cc index 20566b8..41e8f91 100644 --- a/src/tracing/v2/proto_rewriter.cc +++ b/src/tracing/v2/proto_rewriter.cc
@@ -66,7 +66,7 @@ return true; } -// Never leave the caller with the prefix written before the error was found. +// Discard the partial output so the caller cannot use an incomplete packet. RewriteResult Reject(std::vector<uint8_t>* output, RewriteResult result) { PERFETTO_DCHECK(result != RewriteResult::kSuccess); output->clear(); @@ -83,18 +83,24 @@ PERFETTO_CHECK(max_output_size <= UINT32_MAX); output->clear(); - // Open-message stack, stored in output length placeholders: - // - Each slot holds the output offset of its enclosing message's length - // field, or kNoOpenMessage if its parent is the root. - // - Offsets survive output reallocations. - // - A four-byte slot must fit within max_output_size <= UINT32_MAX, so its - // offset is at most UINT32_MAX - 4 and cannot equal kNoOpenMessage. + // Store the stack of open messages in their output length fields: + // 1. On an opening tag, reserve four bytes for the length. Store the parent's + // length-field offset there, or kNoOpenMessage if the parent is the root. + // 2. On a closing byte, read that enclosing offset before replacing it with + // the completed message's length. + // 3. Continue with the enclosing message as the innermost open message. + // + // Offsets remain valid if the output vector reallocates its storage. + // Each four-byte field must fit within max_output_size <= UINT32_MAX. + // Its offset is therefore at most UINT32_MAX - 4 and cannot equal + // kNoOpenMessage. uint32_t innermost_length_offset = kNoOpenMessage; const uint8_t* read_ptr = input_begin; while (read_ptr < input_end) { - // 1. Close the innermost message and backfill its length. 0x04 is a close - // marker only at a field boundary; field parsers consume embedded bytes. + // 1. Close the innermost message and fill its length field. + // Only interpret 0x04 as a closing byte at a field boundary. The parsers + // below consume complete field values, including any embedded 0x04 byte. if (*read_ptr == kProtoGroupEndByte) { if (innermost_length_offset == kNoOpenMessage) return Reject(output, RewriteResult::kMalformedInput); @@ -105,6 +111,7 @@ PERFETTO_DCHECK(kMessageLengthFieldSize <= output->size() - length_offset); uint32_t enclosing_length_offset; + // Save the enclosing offset before the length write overwrites the link. memcpy(&enclosing_length_offset, output->data() + length_offset, sizeof(enclosing_length_offset)); const size_t content_size = @@ -150,7 +157,8 @@ } if (wire_type == kWireTypeEndGroup) { - // Proto-group uses the field-id-less end byte handled above. + // The proto group format requires the single 0x04 closing byte above. + // A standard end-group tag or a multi-byte encoding of 0x04 is invalid. return Reject(output, RewriteResult::kMalformedInput); }
diff --git a/src/tracing/v2/proto_rewriter.h b/src/tracing/v2/proto_rewriter.h index a659372..d8e79ef 100644 --- a/src/tracing/v2/proto_rewriter.h +++ b/src/tracing/v2/proto_rewriter.h
@@ -26,16 +26,16 @@ enum class RewriteResult { kSuccess, - // Not a well-formed proto-group packet: a bad tag, value or length, a stray - // close marker, or a message left open. + // The input has an invalid tag, value or length, an unmatched closing byte, + // or a nested message without a closing byte. kMalformedInput, // The rewritten packet would exceed |max_output_size|, or a nested message // would not fit its four-byte length field. The input itself may be valid. kOutputTooLarge, }; -// Rewrites one packet from tracing v2's append-only proto-group encoding to -// normal, length-delimited protobuf. For example, this nested message: +// Rewrites one packet from tracing v2's append-only proto group encoding to +// standard length-delimited protobuf. For example, this nested message: // // field 1 { // field 2: 7 @@ -43,7 +43,7 @@ // // is encoded and rewritten as follows: // -// proto-group (hex): +// proto group (hex): // 0b 10 07 04 // | | | // start field 1 contents end current message @@ -53,17 +53,22 @@ // | | | // field 1 length 2 (four bytes) contents // -// The root message has no marker. Ordinary protobuf fields are copied -// unchanged. A standard protobuf end-group tag is not a valid proto-group -// close marker. Nesting depth is bounded only by the output size. +// The root message has no marker. The rewriter copies ordinary protobuf fields +// unchanged. A standard protobuf end-group tag is invalid in proto group. +// Each nested message requires a tag and four length bytes in the output, +// so the output size also bounds nesting depth. // -// Requirements: -// - Input is a private copy, outside producer-owned shared memory. -// - Input must not overlap |output|'s storage. -// - |max_output_size| bounds the root and must be <= UINT32_MAX (CHECKed). -// Nesting links use 32-bit offsets. -// - Each nested message must fit its four-byte length field. -// - |output| is empty unless the result is kSuccess. +// Caller requirements: +// - Input must be a private copy, outside producer-owned shared memory. +// Otherwise, the producer can change bytes between validation and the copy. +// - Input must not overlap |output|'s storage, which this function clears. +// - |max_output_size| must be <= UINT32_MAX because nesting links use 32-bit +// output offsets. A larger limit fails a CHECK. +// +// The rewriter limits the entire output to |max_output_size|. Each nested +// message must also fit its four-byte length field. Either limit can produce +// kOutputTooLarge even if the input is valid. +// On failure, |output| is empty. On success, it contains the rewritten packet. RewriteResult RewriteProtoGroupToLengthDelimited(const uint8_t* input_begin, const uint8_t* input_end, std::vector<uint8_t>* output,
diff --git a/src/tracing/v2/proto_rewriter_fuzzer.cc b/src/tracing/v2/proto_rewriter_fuzzer.cc index b8018d8..017f957 100644 --- a/src/tracing/v2/proto_rewriter_fuzzer.cc +++ b/src/tracing/v2/proto_rewriter_fuzzer.cc
@@ -25,9 +25,9 @@ namespace perfetto::tracing_v2 { namespace { -// The bytes come from a producer, so the rewriter has to survive anything. The -// first input byte picks the output bound, so that small inputs reach the -// size-limit paths as well as the malformed ones. +// Check arbitrary producer input for crashes, output overflow, and partial +// output after rejection. The first byte selects the output limit so small +// inputs can exercise both malformed-input and output-limit paths. int FuzzProtoRewriter(const uint8_t* data, size_t size) { if (size == 0) return 0;
diff --git a/src/tracing/v2/proto_rewriter_unittest.cc b/src/tracing/v2/proto_rewriter_unittest.cc index ef5afef..0d18493 100644 --- a/src/tracing/v2/proto_rewriter_unittest.cc +++ b/src/tracing/v2/proto_rewriter_unittest.cc
@@ -212,13 +212,14 @@ })); } -// No per-level state, so protos nest as deep as protozero lets them. +// Length fields store the nesting links, so depth requires no separate stack. +// The output size bounds the supported depth. TEST(ProtoRewriterTest, NestingWellBeyondSixtyFourLevels) { std::vector<uint8_t> out; ASSERT_EQ(Rewrite(NestedTo(65), &out), RewriteResult::kSuccess); // Every level becomes a one-byte tag plus a four-byte length. ASSERT_EQ(out.size(), 65u * 5u); - // The outermost length names everything inside it: 64 levels of 5 bytes. + // The outermost length includes all 64 nested levels, each with 5 bytes. EXPECT_EQ(std::vector<uint8_t>(out.begin(), out.begin() + 5), Bytes({0x0a, 0xc0, 0x82, 0x80, 0x00})); // The innermost is empty. @@ -236,7 +237,7 @@ TEST(ProtoRewriterTest, StandardEndGroupTagIsRejected) { std::vector<uint8_t> out; // 0b = start field 1, 0c = the standard end-group tag for field 1. The - // proto-group closes with 0x04, so this is not a close. + // The proto group closing byte is 0x04, so this is not a close. EXPECT_EQ(Rewrite(Bytes({0x0b, 0x0c}), &out), RewriteResult::kMalformedInput); EXPECT_TRUE(out.empty()); } @@ -355,8 +356,8 @@ EXPECT_EQ(out, legal); } -// Arbitrary producer bytes: any result is fine, and a rejection leaves -// nothing behind. +// Arbitrary producer bytes must not crash the rewriter. A rejection must leave +// the output empty. TEST(ProtoRewriterTest, ArbitraryBytesNeverLeavePartialOutput) { std::vector<uint8_t> out; std::vector<uint8_t> in; @@ -408,7 +409,7 @@ RewriteResult::kOutputTooLarge); EXPECT_TRUE(out.empty()); - // Four input bytes expand to ten. The limit is about the output. + // Four input bytes expand to ten output bytes, which determines the limit. EXPECT_EQ(Rewrite(Bytes({0x0b, 0x13, 0x04, 0x04}), &out, /*max_output=*/10), RewriteResult::kSuccess); EXPECT_EQ(out.size(), 10u); @@ -416,7 +417,7 @@ RewriteResult::kOutputTooLarge); EXPECT_TRUE(out.empty()); - // A scalar that does not fit after a nested message already went out. + // The nested message fits, but the following scalar exceeds the output limit. EXPECT_EQ(Rewrite(Bytes({0x0b, 0x04, 0x08, 0x01}), &out, /*max_output=*/6), RewriteResult::kOutputTooLarge); EXPECT_TRUE(out.empty()); @@ -474,8 +475,8 @@ ASSERT_EQ(decoded.for_testing().payload().str().size(), 1u); EXPECT_EQ(decoded.for_testing().payload().str()[0], "nested"); - // The private bytes are not a valid protobuf on their own: the whole point is - // that they need this pass. + // The private closing bytes are invalid in standard protobuf. Decode only + // after the rewriter replaces the nested-message framing. protos::gen::TracePacket not_decoded; EXPECT_FALSE( not_decoded.ParseFromArray(private_bytes.data(), private_bytes.size()));
diff --git a/src/tracing/v2/trace_writer_v2_impl.cc b/src/tracing/v2/trace_writer_v2_impl.cc index 6ec26e5..f42472a 100644 --- a/src/tracing/v2/trace_writer_v2_impl.cc +++ b/src/tracing/v2/trace_writer_v2_impl.cc
@@ -33,7 +33,7 @@ namespace perfetto::tracing_v2 { namespace { -// Avoid starting a fragment without room for protozero's largest scalar field. +// A new fragment must fit Protozero's largest scalar field. constexpr uint32_t kMinFragmentPayloadSize = protozero::proto_utils::kMaxSimpleFieldEncodedSize; @@ -53,15 +53,17 @@ TraceWriterV2Impl::~TraceWriterV2Impl() { FinishTracePacket(); - // Publish the last chunk before the delegate starts retiring this WriterID. + // Publish the last chunk before the delegate starts retirement of this + // WriterID. The reader can still need its resources after destruction. ring_buffer_writer_.FinishCurrentChunk(); stream_writer_.Reset({nullptr, nullptr}); delegate_->OnWriterDestroyed(writer_id()); } TraceWriter::TracePacketHandle TraceWriterV2Impl::NewTracePacket() { - // Direct Message::Finalize() bypasses the handle's finalization callback. - // Close the previous packet's fragment before starting another packet. + // A caller can invoke Message::Finalize() directly, without the handle's + // finalization callback. The message then closes, but packet_open_ stays true + // and its fragment still needs publication. Finish that packet before reuse. if (packet_open_ && cur_packet_->is_finalized()) FinishTracePacket(); PERFETTO_CHECK(!packet_open_); @@ -76,6 +78,8 @@ stream_writer_.Reset(EnterDropMode()); } + // Reset selects proto group for the root. Each BeginNestedMessage() call + // inherits its parent's encoding, so all children use append-only framing. cur_packet_->Reset(&stream_writer_, protozero::NestedMessageEncoding::kProtoGroup); packet_open_ = true; @@ -93,24 +97,28 @@ if (!packet_open_) return; + // Finalize() can append closing bytes that cross a chunk boundary. Keep + // packet_open_ true until it returns so GetNewBuffer() can obtain space for + // those bytes. Then publish the final fragment without a continuation flag. cur_packet_->Finalize(); packet_open_ = false; ClosePacketFragment(/*continues_on_next=*/false); - // Failed chunk claims may have left earlier reservations unclaimed. - // The reader must consume those positions before reaching this packet. + // Failed chunk claims can leave earlier reservations unclaimed. Notify the + // reader so it can consume those positions and this packet's fragments. delegate_->NotifyReader(); } void TraceWriterV2Impl::Flush(std::function<void()> callback) { - // Close any fragment left open by direct Message::Finalize() before flushing. - // Raw stream callers must still call FinishTracePacket() themselves. + // Direct Message::Finalize() closes the message without the handle callback. + // Publish its remaining fragment before the flush. Raw stream callers bypass + // message finalization, so they must call FinishTracePacket() themselves. if (packet_open_ && cur_packet_->is_finalized()) FinishTracePacket(); PERFETTO_CHECK(!packet_open_); - // A completed fragment leaves its chunk cached so that the next packet can - // append to it. Release it before delegating flush completion. + // A completed fragment leaves its chunk cached for later packets. Release + // that chunk before Delegate::Flush() so later packets use new reservations. ring_buffer_writer_.FinishCurrentChunk(); stream_writer_.Reset({nullptr, nullptr}); delegate_->Flush(writer_id(), std::move(callback)); @@ -122,8 +130,8 @@ protozero::ContiguousMemoryRange TraceWriterV2Impl::GetNewBuffer() { if (!packet_open_) { - // protozero asked for space outside a packet. That only happens if a - // caller writes through a stale handle, which is a data-source bug. + // The caller requested space without an open packet, for example through + // a stale handle. This violates the data-source API contract. PERFETTO_DFATAL("TraceWriterV2Impl: write outside an open packet"); return EnterDropMode(); } @@ -134,15 +142,20 @@ if (in_drop_mode_) return EnterDropMode(); - // The packet did not fit. Publish this fragment with ContinuesOnNext and - // continue the packet in another chunk. + // The current range cannot satisfy the stream's request. Publish this + // fragment with kFlagContinuesOnNextChunk and continue in another chunk. ClosePacketFragment(/*continues_on_next=*/true); - // Notify before reserving the continuation. A packet can fill the entire - // ring buffer without finishing. A stalling writer would wait for space - // before the reader had been asked to release any. + // Notify before the next reservation. Otherwise, a packet larger than the + // ring buffer can stall before the reader receives any notification: + // 1. The writer fills the ring buffer without completing the packet. + // 2. The next fragment needs space, so the writer waits for the reader. + // 3. The reader has no notification to consume the published fragments. delegate_->NotifyReader(); + // ClosePacketFragment() can fail to relocate after the reader requests a + // rewrite. + // In that case, discard the remainder instead of reserving a continuation. if (in_drop_mode_) return EnterDropMode(); @@ -170,9 +183,14 @@ static_cast<uint32_t>(stream_writer_.write_ptr() - fragment_begin_); fragment_begin_ = nullptr; - // A failed relocation leaves this packet incomplete. Finish it in the drop - // buffer. SharedRingBufferWriter flags the loss on its next publication. + // EndFragment() can need space even though this fragment already has a chunk: + // 1. The reader copies the published fragments while this fragment is open. + // 2. It sets RewriteRequested and advances past the reservation. + // 3. EndFragment() cannot publish here. It saves this fragment, acknowledges + // the request, and tries to publish the fragment in another chunk. const auto result = ring_buffer_writer_.EndFragment(used, continues_on_next); + // On failure to acquire that chunk, discard this packet's remaining bytes. + // SharedRingBufferWriter reports the loss on its next publication. if (result == SharedRingBufferWriter::EndFragmentResult::kRelocationDropped) { in_drop_mode_ = true; ++drop_count_; @@ -182,8 +200,8 @@ protozero::ContiguousMemoryRange TraceWriterV2Impl::EnterDropMode() { fragment_begin_ = nullptr; - // A failed relocation may already have set in_drop_mode_. Allocate on the - // first use independently of how the packet entered drop mode. + // A relocation failure can set in_drop_mode_ before the first use of this + // buffer. Check the buffer itself so that path also allocates storage. if (drop_buffer_.empty()) drop_buffer_.resize(ring_buffer_writer_.chunk_size());
diff --git a/src/tracing/v2/trace_writer_v2_impl.h b/src/tracing/v2/trace_writer_v2_impl.h index 409390a..fd386c7 100644 --- a/src/tracing/v2/trace_writer_v2_impl.h +++ b/src/tracing/v2/trace_writer_v2_impl.h
@@ -46,22 +46,25 @@ // TraceWriter implementation backed by a tracing v2 shared ring buffer. // // A packet normally occupies one fragment. If it crosses a chunk boundary, the -// writer sets the continuation flags and carries on in the next chunk. The +// writer sets the continuation flags and continues in the next chunk. The // reader joins those fragments to reassemble the packet. // // A nested message can span chunks and still be open when the reader copies // an earlier fragment. The two encodings handle this differently: // -// - Length-delimited (v1): Protozero reserves a length field before each nested -// message and fills it in when the message finishes. If the chunk containing -// that field must be released first, TraceWriterImpl redirects the length -// write to a patch record. Once the message finishes, the arbiter sends the -// completed patch to the service to update its copy of the chunk. +// - Length-delimited (v1): +// 1. Protozero reserves a length field before each nested message. +// 2. If the chunk must be released before the message ends, TraceWriterImpl +// redirects the length write to a patch record. +// 3. When the message ends, Protozero writes its length. The arbiter sends +// any completed patch to the service, which updates its copy of the chunk. // -// - Proto-group (this writer): Each nested message starts with a group tag and -// ends with an appended closing byte. Previously published bytes need no -// length update. After reassembly, ProtoRewriter converts the packet to -// ordinary length-delimited protobuf. +// - Proto group (this writer): +// 1. Each nested message starts with a group tag. +// 2. When the message ends, Protozero appends a closing byte. Previously +// published bytes need no length update. +// 3. After reassembly, ProtoRewriter converts the packet to ordinary +// length-delimited protobuf. // // If the writer drops any part of a packet: // - Its remaining bytes go to the drop buffer. @@ -70,8 +73,8 @@ // - Packet reassembly discards orphan continuations. Later complete packets // in unflagged chunks can be delivered. // -// As with TraceWriterImpl, an instance may be used by only one thread at a -// time. Packet writes go straight to the ring buffer. The delegate handles +// As with TraceWriterImpl, use each instance from one thread at a time. +// Packet writes go directly to the ring buffer. The delegate handles // reader notifications, flush completion and WriterID retirement. class TraceWriterV2Impl : public TraceWriter, public protozero::MessageFinalizationListener, @@ -90,29 +93,32 @@ // v // Delegate // - // Several writers can share this delegate. Each keeps it alive for as long - // as it needs it, independently of when the other writers are destroyed. + // Each writer retains its own reference until destruction. Destroying one + // writer must not destroy a delegate that other writers still use. // // Refcounting manages lifetime only. Each writer still needs a single // calling thread or external synchronization. // - // Calls run on the writer's thread and may overlap across writers. + // Calls run on each writer's thread and can overlap across writers. // Implementations must synchronize access to their shared state. class Delegate : public SharedRingBufferWriter::Delegate { public: ~Delegate() override; // Called after this writer publishes its data. For a non-empty callback: - // - Wait for service acknowledgement before invoking it. + // - Invoke it after service acknowledgement. // - The delegate chooses the callback sequence. - // - Disconnect may discard it, as allowed by TraceWriter::Flush(). + // - On disconnect, the delegate can discard it, as TraceWriter::Flush() + // permits. virtual void Flush(WriterID, std::function<void()> callback) = 0; // The caller owns the writer, while the writer retains this delegate. - // Releasing that reference does not tell the delegate which writer ended. - // Called after this writer publishes its final chunk. The delegate decides - // when the reader no longer needs the writer's resources and its WriterID - // can be used by another writer. + // Reference release alone does not identify which writer ended. + // OnWriterDestroyed() supplies that identity after the final publication. + // + // The reader can still have unconsumed positions for this writer. The + // delegate must retain its resources until the reader no longer needs them. + // Only then can another writer use this WriterID. // // Like v1, where ~TraceWriterImpl() flushes remaining data and calls // SharedMemoryArbiterImpl::ReleaseWriterID(): @@ -122,17 +128,20 @@ // publish last chunk // OnWriterDestroyed(id) -----------> arrange retirement of id // - // Prototype note: InProcessTracingV2Bridge forwards v2 ring data through a - // retained v1 writer. It waits for the reader to pass the final publication - // before releasing that writer and its reassembly state, so unread packets - // can still be forwarded. Once we have validated the v2 ring buffer and the - // service reads it directly, check whether writer cleanup can be simpler - // without the bridge. + // In the prototype, InProcessTracingV2Bridge forwards v2 packets through a + // retained v1 writer: + // 1. The v2 writer publishes its final chunk and calls OnWriterDestroyed(). + // 2. The reader consumes the remaining positions. The bridge still needs + // the v1 writer and reassembly state to forward those packets. + // 3. After the reader passes the final publication, the bridge can release + // the v1 writer and reassembly state. + // Revisit this cleanup when the service reads the v2 ring buffer directly + // and the bridge is no longer needed. virtual void OnWriterDestroyed(WriterID) = 0; }; struct InitArgs { - // Must be non-null. Retained until the writer is destroyed. + // Must be non-null. Each writer retains a reference until its destruction. std::shared_ptr<Delegate> delegate; // Identifies this writer and the trace buffer its packets target. WriterID writer_id = 0; @@ -188,17 +197,19 @@ std::unique_ptr<protozero::RootMessage<protos::pbzero::TracePacket>> cur_packet_; - // Start of the fragment currently being filled. Null while writes are going - // to |drop_buffer_|. + // Start of the open fragment in shared memory. Null after the fragment closes + // and while writes use |drop_buffer_|. uint8_t* fragment_begin_ = nullptr; - // When a packet is dropped, protozero writes its remaining bytes here so - // the data source can finish writing normally. These bytes are discarded. + // If a packet is dropped, Protozero writes its remaining bytes here so the + // data source can finish the packet. These bytes are never published. + // Each writer needs its own buffer because writes use ordinary stores. + // A global buffer would allow concurrent writers to modify the same bytes. // // Allocate this buffer only when needed, then reuse it for later drops. // Writers that never drop avoid the extra memory. The tradeoff is an - // allocation on the first drop. Use one chunk so raw stream callers can - // still reserve a contiguous range of bytes. + // allocation on the first drop. Match the chunk size so raw stream callers + // can still reserve a contiguous range as large as a normal fragment. std::vector<uint8_t> drop_buffer_; bool packet_open_ = false;
diff --git a/src/tracing/v2/trace_writer_v2_impl_unittest.cc b/src/tracing/v2/trace_writer_v2_impl_unittest.cc index 926e5f5..eabef96 100644 --- a/src/tracing/v2/trace_writer_v2_impl_unittest.cc +++ b/src/tracing/v2/trace_writer_v2_impl_unittest.cc
@@ -64,8 +64,8 @@ std::vector<WriterID> destroyed_writers; }; -// Drains the ring buffer synchronously from the notification to test -// notification timing deterministic. +// Drains the ring buffer synchronously on notification so tests can verify +// the notification order without thread scheduling. class DrainingDelegate : public TraceWriterV2Impl::Delegate { public: void NotifyReader() override { @@ -83,9 +83,9 @@ uint32_t notifications = 0; }; -// Puts the writer's fragments back together into packets and rewrites them to -// canonical protobuf. Uses fragment order, continuation flags and loss reports -// to check that only complete packets reach the decoder. +// Reassembles the writer's fragments into packets and rewrites them to +// length-delimited protobuf. Uses fragment order, continuation flags and loss +// reports to check that only complete packets reach the decoder. class PacketReassembler : public SharedRingBufferReader::Delegate { public: void OnChunkRead( @@ -187,9 +187,12 @@ Fixture f(/*num_chunks=*/2, /*chunk_size=*/256); f.writer->NewTracePacket()->set_timestamp(1); - // Occupy the other chunk, then request a rewrite of the writer's cached - // chunk. Model the reader paused before it advances read_pos, so the writer - // cannot reserve a replacement for its unpublished fragment. + // Force the first drop through relocation failure: + // 1. A second writer occupies the other chunk. Both positions are reserved. + // 2. The reader requests a rewrite of this writer's chunk, then pauses before + // it advances read_pos. The ring buffer still appears full to the writer. + // 3. EndFragment() acknowledges the request but cannot reserve a replacement. + // GetNewBuffer() must allocate the drop buffer for the remaining bytes. SharedRingBufferWriter blocker(f.ring.get(), 9, kBufferA, BufferExhaustedPolicy::kDrop, test::GetNoopWriterDelegate()); @@ -204,7 +207,7 @@ ASSERT_TRUE(f.ring->TryRequestRewrite(ChunkIndex::FromIndex(0), &observed)); const std::string payload(1024, 'x'); packet->AppendRawProtoBytes(payload.data(), payload.size()); - // Relocation records the drop before GetNewBuffer calls EnterDropMode. + // Relocation records the drop before GetNewBuffer() calls EnterDropMode(). // Allocation must happen even though in_drop_mode_ is already true. EXPECT_EQ(f.ring->LoadChunkStateWordAcquire(ChunkIndex::FromIndex(0)), kRewriteAcknowledgedStateWord); @@ -362,8 +365,9 @@ f.writer->Flush(); f.reader.Drain(64); - // Proto-group packets are not directly parseable as protobuf. They become - // ordinary protobuf after rewriting. + // The proto group closing bytes make this packet invalid as standard + // protobuf. The rewriter converts the nested messages to length-delimited + // fields. ASSERT_EQ(f.reassembler.packets.size(), 1u); const std::vector<uint8_t>& raw = f.reassembler.packets[0]; EXPECT_NE(std::find(raw.begin(), raw.end(), @@ -383,8 +387,8 @@ } TEST(TraceWriterV2ImplTest, PacketSpanningSeveralChunksReconstructsExactly) { - // 256-byte chunks give about 249 payload bytes, so a 2 KiB string has to be - // split across many of them. + // A 256-byte chunk holds at most 248 payload bytes after its header and size + // entry. The 2 KiB string must therefore span several chunks. Fixture f(/*num_chunks=*/64, /*chunk_size=*/256); const std::string payload(2048, 'x'); { @@ -406,10 +410,12 @@ TEST(TraceWriterV2ImplTest, FinalizingAPacketCanCrossAChunkBoundary) { Fixture f(/*num_chunks=*/8, /*chunk_size=*/256); - // An empty chunk holds a 248-byte fragment. The three-byte first-packet - // marker, two-byte opening tag for TracePacket.for_testing, string tag, - // two-byte length and payload fill it exactly. Finalize() has to write the - // nested-message close byte into another chunk. + // A 256-byte chunk holds a 248-byte fragment after its header and size entry. + // Fill that fragment exactly: + // - 3 bytes for first_packet_on_sequence. + // - 2 bytes for the opening tag of TracePacket.for_testing. + // - 1 byte for the string tag, 2 for its length, and 240 for its payload. + // Finalize() must then append the nested-message closing byte in a new chunk. const std::string payload(240, 'z'); auto packet = f.writer->NewTracePacket(); packet->set_for_testing()->set_str(payload); @@ -430,8 +436,8 @@ // kDrop would lose the remaining bytes. kStall would wait for a reader // that was never told to run. // -// kDrop deliberately, so that the failure is a wrong answer in milliseconds -// rather than a 30-second stall. +// Use kDrop so a missing notification fails through lost data without a +// 30-second stall. TEST(TraceWriterV2ImplTest, PacketLargerThanTheWholeRingSurvives) { test::SharedRingBufferForTesting ring(/*num_chunks=*/2, /*chunk_size=*/256); PacketReassembler reassembler; @@ -479,8 +485,8 @@ SharedRingBufferReader reader(ring.get(), &reassembler); auto delegate = std::make_shared<CountingDelegate>(); - // Both chunks pinned by a writer that stopped mid-rewrite, so no claim the - // writer below makes can succeed. + // Both chunks await rewrite acknowledgement from an earlier writer. + // The new writer cannot claim either chunk until the reader reclaims it. const uint32_t being_written = MakeDataStateWord(ChunkState::kBeingWritten, ChunkFormat::kTargetBuffer, 0, 0, kWriterA + 1); @@ -502,7 +508,7 @@ TraceWriter::TracePacketHandle packet = writer.NewTracePacket(); packet->set_timestamp(1); - // Still open, and the reader has already been told. + // The packet is still open, but the reader already received a notification. EXPECT_GT(delegate->notifications, 0u); EXPECT_GT(ring->LoadWritePosRelaxed(), 0u); } @@ -554,9 +560,9 @@ EXPECT_EQ(packets[1].timestamp(), 2u); } -// The reader takes the chunk while a packet is still being written. The writer -// relocates only the part it had not published, and the packet still comes out -// once and whole. +// The reader copies the first packet and requests a rewrite while the writer +// fills the second packet. The writer relocates only that unpublished packet. +// The reader must deliver both packets exactly once. TEST(TraceWriterV2ImplTest, ScrapeDuringAnOpenPacketProducesOneCanonicalPacket) { Fixture f(/*num_chunks=*/16, /*chunk_size=*/512);