tracing: add append-only proto encoding for v2 writers - #7325
Conversation
🎨 Perfetto UI Builds
|
673a5c4 to
1d1afad
Compare
2f34eb2 to
455aac3
Compare
455aac3 to
3a756b6
Compare
3a756b6 to
6bc6694
Compare
6bc6694 to
287be4c
Compare
Protozero normally back-patches nested-message lengths. The v2 ring can expose committed prefixes before their enclosing messages close, so packets written there need append-only framing. - Add opt-in ProtoGroup framing to the C and C++ Protozero APIs while keeping length-delimited protobuf as the default. - Use one private C++ framing discriminator for length-delimited messages, ProtoGroup roots and nested ProtoGroup messages without growing Message. - Preserve the public C message layout and reject incremental STRING and PACKED fields in ProtoGroup mode until buffered construction is needed. - Carry the selected encoding through C data-source packet creation while keeping the legacy entry point restricted to length-delimited writers. - Pin the C++ Message size to its previous 32-bit and 64-bit debug and release budgets. - Rewrite copied ProtoGroup packets into ordinary length-delimited protobuf, validating the wire format and bounding output and nesting links. - Implement TraceWriterV2 fragmentation, continuation, loss reporting and append-only finalization on SharedRingBufferWriter. - Make the retained TraceWriterV2 delegate provide the ring so its storage and coordination share one lifetime contract. - Test exact C and C++ encodings, malformed input, output limits, chunk-boundary finalization, scraping, drops, direct stream reservations, delegate retention and unchanged defaults.
287be4c to
f015a42
Compare
primiano
left a comment
primiano
left a comment
There was a problem hiding this comment.
I have some limited time today but i'm sending some intiial comments.
I only looked at message.h/cc, trace_writer_v2.h/cc, which I believe are the more intetesting ones and worth getting comments sooner.
I didn't look at the rewriter, but givenm teh signature I suppose it's non controversial, to the point where I'd blindly trust a decent AI Agent to deal with that.
I dind't look at the shared_lib stuff because i'm not fmailiar with it and woudl take me too time today to look. but again I don't think there is a lot of juicy stuff there
| // | ||
| // In proto-group mode, a nested message ends with a closing byte and the | ||
| // root ends at the packet boundary. Neither writes a length before its | ||
| // contents, so there is no length field to fill in later. |
There was a problem hiding this comment.
I'm assuming that in group mode the size_field is always null?
In this case make the comemnt stronger. Say "this is unused and always null in group mode".
Or maybe it's set but ignored (which is also fine). but then say which one is which.
EDIT after reading the message.cc looks like it's always null.
| // END closes the child. No additional byte closes the root. | ||
| // Both messages use the same encoding, so we need is_root_ to tell them | ||
| // apart. | ||
| bool is_root_; |
There was a problem hiding this comment.
I'm going to be a bit handwavy here, but I wonder if we can leverage the fact that we have already the notion of a RootMessage (for arena purposes).
TraceWritrer (and also your V2) has a reference to a root message.
So I am thinking something on the lines of:
- We make Message::Finalize take an optional argument, so Finalize(bool is_root=false)
- We override (without making it virtual) the Finalize method in RootMessage (keeping it a void)
so something like:
class RootMessage : public T {
...
Finalize() {
Finalize(/*is_root=*/true);
}
}
I think this shoudl have the same effect without adding further state tracking.
But I have not tought deeply to this, so don't take it as granted.
I wouldn't be surprised if you found some good reason why we have to do your way.
|
|
||
| // Write the proto preamble for the nested message. | ||
| uint32_t tag; | ||
| switch (encoding_) { |
There was a problem hiding this comment.
Imho the switch(encoding_) is 1. overkill, 2. inconsistent with what you did above (where you had an if(UNLIKELY)). Pick one and be consistent.
I'd honestly just go with a plain if.
siwtch adds more nesting, and sometimes causes problems with older compilers about "what if it's option 3".
| // Wire types 3 and 4 delimit a protobuf group. Protozero decoders do not | ||
| // support groups, so keep them out of ProtoWireType. Tracing v2 only borrows | ||
| // the start-group wire type for the encoding below. | ||
| constexpr uint32_t kWireTypeStartGroup = 3; |
There was a problem hiding this comment.
When commenting on kWireTypeStartGroup explain that we bastardized group (okay not in those terms) and don't follow the official encoding for it (you can link at the RFC)
| // As with TraceWriterImpl, an instance may be used by only one thread at a | ||
| // time. Packet writes go straight to the ring; the delegate handles reader | ||
| // notifications, flush completion and WriterID retirement. | ||
| class TraceWriterV2 : public TraceWriter, |
There was a problem hiding this comment.
Maybe purely for naming consistency (when comparing and doing codesearch) this should be called TraceWriterV2Impl, as thi feeels the moral equivalent of TraceWriterImpl, right?
|
|
||
| struct InitArgs { | ||
| // Must be non-null. Retained until the writer is destroyed. | ||
| std::shared_ptr<Delegate> delegate; |
There was a problem hiding this comment.
out of curiosity why the Deletgate is a shared_ptr and not a unique_ptr?
A TraceWriter is not thread-safe (it's supposed to be used on the same thread, or caller must use a lock), so I'm not sure on why you need the refcounting here?
|
|
||
| // Called after the writer publishes its last chunk. The WriterID must not | ||
| // be reused while unconsumed ring positions can still name it. | ||
| virtual void OnWriterDestroyed(WriterID) = 0; |
There was a problem hiding this comment.
uh why the delegate has this notirication? isn't the owner of the delegate supposed to know when the writer is destoryed?
I think I don't fully understand how ownership/lifecycle works for TWv2
| cur_packet_(new protozero::RootMessage<protos::pbzero::TracePacket>()) { | ||
| // Protozero never receives more than one chunk at a time. Reserving the same | ||
| // amount here keeps the drop path allocation-free. | ||
| drop_buffer_.resize(delegate_->ring_buffer().chunk_size()); |
There was a problem hiding this comment.
can we lazily allocate this if we ever need it, rather than pre-serving one chunk per thread? feels too aggressive memory-wise.
Or actually can the drop buffer be one for all (i.e. a global in the anonymous name space here) rather than one per writer?
is it like the garbage chunk? or do we care about ownerhsip of the writes ? is this going to do CAS & co on this drop_buffer?
| // Publish the last chunk before notifying the delegate of destruction. | ||
| ring_buffer_writer_.FinishCurrentChunk(); | ||
| stream_writer_.Reset({nullptr, nullptr}); | ||
| delegate_->OnWriterDestroyed(writer_id()); |
There was a problem hiding this comment.
isn't the delegate the same obhect that owns TraceWriterV2 (which means it could tell precisely when it's going to be destroyed?)
| stream_writer_.Reset(EnterDropMode()); | ||
| } | ||
|
|
||
| // Every nested message inherits the root's encoding. |
There was a problem hiding this comment.
i'm not sure I understood this comment
There is a slilghtly controversial thing in include/perfetto/public/pb_msg.h , I would like your opinion on. About how I am (ab)using PerfettoPbMsg::size field |
2 participants
Protozero normally back-patches nested-message lengths. The v2 ring can expose committed prefixes before their enclosing messages close, so packets written there need append-only framing.
Add opt-in ProtoGroup framing to the C and C++ Protozero APIs while keeping length-delimited protobuf as the default.
Use one private C++ framing discriminator for length-delimited messages, ProtoGroup roots and nested ProtoGroup messages without growing Message.
Preserve the public C message layout and reject incremental STRING and PACKED fields in ProtoGroup mode until buffered construction is needed.
Carry the selected encoding through C data-source packet creation while keeping the legacy entry point restricted to length-delimited writers.
Pin the C++ Message size to its previous 32-bit and 64-bit debug and release budgets.
Rewrite copied ProtoGroup packets into ordinary length-delimited protobuf, validating the wire format and bounding output and nesting links.
Implement TraceWriterV2 fragmentation, continuation, loss reporting and append-only finalization on SharedRingBufferWriter.
Make the retained TraceWriterV2 delegate provide the ring so its storage and coordination share one lifetime contract.
Test exact C and C++ encodings, malformed input, output limits, chunk-boundary finalization, scraping, drops, direct stream reservations, delegate retention and unchanged defaults.