tp: let TraceBlob wrap memory it does not own - #7269
Draft
LalitMaganti wants to merge 6 commits into
Draft
Conversation
🎨 Perfetto UI Builds
|
sashwinbalaji
approved these changes
Aug 28, 2026
| MaybePrintProgress(); | ||
|
|
||
| if (len == 0) | ||
| if (blob.size() == 0) |
Member
There was a problem hiding this comment.
nit: this can be len(as before)
LalitMaganti
force-pushed
the
dev/lalitm/zerocopy-5-traceblob-adopt
branch
from
August 28, 2026 14:30
2cedaa0 to
70cbf79
Compare
Every transport kept a staging buffer, read into it, then handed those
bytes to Append(), which copied them again. BeginWrite()/EndWrite() lets
read(2) deposit them in the ring buffer directly, so they are copied
once.
The reservation is handed out as a move-only WriteHandle that must be
consumed exactly once, by EndWrite() to keep what was written or
AbortWrite() to discard it; dropping one fails a CHECK. Holding it in a
value the compiler tracks is what makes the pattern safe to use: the
buffer refuses to recompact, grow or hand out a second reservation while
one is outstanding, so bytes cannot move under a pointer someone still
holds. Rpc::RequestHandle carries the same contract out to the
transports.
Four bugs fell out of this, three of them a reservation abandoned on a
path that returns early:
- The size of the reservation used to live in a member alongside the
pointer, and the path that gives up on a stream too long to ever form
a message returned without setting it. The caller's next EndWrite()
then failed its CHECK, so a peer could abort the process by sending
128MB of varint continuation bytes. The count now lives in the handle,
where it cannot fall out of step with the pointer it describes.
- traceconv dropped the reservation it sniffs the first chunk in
whenever the trace turned out to be compressed, and again whenever the
decompressor errored.
- RemoteTraceProcessor dropped one whenever a recv failed.
unixd additionally re-serialized each tokenized message back into its
TraceProcessorRpcStream framing and pushed it through a second ring
buffer inside Rpc. Rpc::OnRpcMessage() dispatches it as-is.
out/linux_clang_release/perfetto_benchmarks \
--benchmark_filter='ProtoRingBufferIngest|ProtoRingBufferDispatch'
ingest, 4KB reads 142 ns -> 98.3 ns
ingest, 1MB reads 38.4 us -> 19.2 us
dispatch 171 ns -> 99.1 ns
traceconv also drops a 64MB read buffer: peak RSS 62.7MB -> 13.6MB.
trace_processor_rpc_init() allocated a 32MB request buffer that JS wrote into and C++ copied out of on every chunk. The tokenizer holds a standing reservation instead and hands back the address for the next write, so nothing is copied and it stays at one JS->Wasm call per request. Entry points are resolved once with cwrap() instead of ccall(), which redoes the symbol lookup and argument marshalling on every call. For a numeric signature cwrap() returns the raw wasm export, so this is as direct as it gets: 29.6 ns -> 4.5 ns per call. memory64, vs the staging buffer: trace load -61.5%, queries -23.7% addFunction() was declared as returning void; it returns a function pointer. ccall()'s any[] hid that, cwrap()'s number[] does not.
Rpc has, until now, held a single tokenizer and a single pair of
{tx,rx} sequence ids for the whole process, even though several
transports serve more than one peer at a time. That state is per-pipe,
not per-TraceProcessor, and sharing it is visibly broken: two clients
connected to --httpd at once knock each other off the channel with
RPC request out of order. Expected 2, got 1 (ERR:rpc_seq)
as soon as the second one sends its second request, because each peer
numbers its requests from its own 0.
unixd.cc had already worked around its half of this by keeping a
ProtoRingBuffer per connection and forwarding only whole messages, which
fixed framing for its clients but not the sequence ids, and left the
same workaround waiting to be written again in httpd.cc.
Introduce Rpc::Stream, which owns exactly the state that belongs to one
byte-pipe: the tokenizer holding a message that arrived in pieces, the
sequence ids, and the response function. The TraceProcessor instance
stays shared. Transports that serve one peer (Wasm, stdiod) hold one
stream; those that serve many (httpd, unixd) hold one per connection.
This also retires the SetRpcResponseFunction(fn)/SetRpcResponseFunction
(nullptr) dance every transport had to perform around each dispatch: the
response function is now handed to the stream once, at construction.
unixd loses its hand-rolled tokenizer and message-forwarding loop, as
the stream does that now.
Note this isolates framing only. Streams still share one TraceProcessor,
so two peers can trip over each other's trace state; drawing a session
boundary is a separate problem.
This makes it such that instead of the HttpServer providing the buffer for HTTP/Websocket payloads, it instead asks the handler for where they would like the payload to be stored. The flow looks like this: socket read -> HTTPServer reads the headers/framing -> once it knows the size it calls OnHttpRequestBody or OnWebsocketPayload depending on the message type -> the handler provides the storage bytes -> httpserver reads into those provided buffers. The motivation for this is that it removes one copy allowing for "zero-copy" parsing and tokenization etc. The motivation for this is multi-threaded trace processor which requires fast-handoffs of buffers between threads and reducing the latency of the parse path (because we're going to be adding latency by a cross thread hop). This change is one step on that journey. Because a payload is now read across several socket reads, more than one can be in flight at a time, so every buffer the handler hands out has to belong to the connection asking for it. RPC payloads go into that connection's Rpc::Stream, added in the previous change; the bodies of the non-RPC endpoints go into a per-connection buffer alongside it. Sharing either across connections lets one connection's payload be dispatched into another's, or freed under it when the buffer grows. The origin check moves above the point where the body is requested, so a request that is about to be refused with a 403 is never handed a payload sink to write into. The reservation is held in the connection's state between the two calls, so a peer that disappears mid-payload gives it back explicitly rather than leaving the tokenizer believing a write is still in flight.
Append()'s fastpath returned messages pointing into the caller's buffer, valid only until the next call. That is why RemoteTraceProcessor had to hoist its read buffer out of the loop, and it is what stops a Message from owning its storage. Every caller now writes through BeginWrite(), so both can go. Also collapses RingBufferMessageReader into ProtoRingBuffer: the virtual had a single production implementation, and a test-only subclass whose coverage the ProtoRingBuffer tests subsume. The benchmarks added to motivate the switch go too: their "before" arm is the staging buffer this removes.
TraceBlob can hold heap memory it allocated or an mmap region, so a caller that already has the bytes somewhere else has no way to hand them over: it has to copy them into a blob first. Add an ownership mode that wraps a pointer and holds a shared_ptr to whoever owns it, for as long as the blob and every TraceBlobView onto it. Give Rpc::Parse() an overload taking a TraceBlobView so callers can use it; the pointer-and-length overload stays and now copies through TraceBlob::CopyFrom() rather than open-coding new[] and memcpy. No behaviour change: nothing adopts anything yet.
LalitMaganti
force-pushed
the
dev/lalitm/zerocopy-5-traceblob-adopt
branch
from
August 28, 2026 15:10
70cbf79 to
42f39cc
Compare
LalitMaganti
marked this pull request as draft
August 28, 2026 15:34
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TraceBlob can hold heap memory it allocated or an mmap region, so a
caller that already has the bytes somewhere else has no way to hand
them over: it has to copy them into a blob first.
Add an ownership mode that wraps a pointer and holds a shared_ptr to
whoever owns it, for as long as the blob and every TraceBlobView onto
it. Give Rpc::Parse() an overload taking a TraceBlobView so callers can
use it; the pointer-and-length overload stays and now copies through
TraceBlob::CopyFrom() rather than open-coding new[] and memcpy.
No behaviour change: nothing adopts anything yet.