tp: integrate ProtoVM by rukkal · Pull Request #4768 · google/perfetto

rukkal · 2026-02-11T19:08:03Z

Integrate ProtoVM into TP as specified in go/perfetto-proto-vm.

github-actions · 2026-02-11T19:17:29Z

🎨 Perfetto UI Builds

c03b7d4 (2026-02-11): https://storage.googleapis.com/perfetto-ci-artifacts/gh-21919168372-1-ui/ui/index.html
1768c4d (2026-02-12): https://storage.googleapis.com/perfetto-ci-artifacts/gh-21948217116-1-ui/ui/index.html
1768c4d (2026-02-12): https://storage.googleapis.com/perfetto-ci-artifacts/gh-21948217116-2-ui/ui/index.html
1768c4d (2026-02-12): https://storage.googleapis.com/perfetto-ci-artifacts/gh-21948217116-3-ui/ui/index.html
1768c4d (2026-02-12): https://storage.googleapis.com/perfetto-ci-artifacts/gh-21948217116-4-ui/ui/index.html
3bb7257 (2026-02-13): https://storage.googleapis.com/perfetto-ci-artifacts/gh-21980931271-1-ui/ui/index.html
19d8fd3 (2026-02-13): https://storage.googleapis.com/perfetto-ci-artifacts/gh-21987795789-1-ui/ui/index.html
8a0ce10 (2026-02-16): https://storage.googleapis.com/perfetto-ci-artifacts/gh-22069243941-1-ui/ui/index.html
73c61b3 (2026-02-16): https://storage.googleapis.com/perfetto-ci-artifacts/gh-22070020341-1-ui/ui/index.html
ca180c7 (2026-02-17): https://storage.googleapis.com/perfetto-ci-artifacts/gh-22100072787-1-ui/ui/index.html

…ently This CL introduces a new class to write TracePacket protos efficiently and get out TraceBlobViews without creating tons of copies. There were already N instances of this in the codebase and we're now introducing a new one in #4768. Instead now we have a single class which works with TraceBlob + TraceBlobView and is stored in TraceProcessorContext so everyone can use it. It works in 4MB chunks and reuses them across TracePacket calls which makes it really efficient now to create these packets with 99% of cases being totally zero-copy.

src/trace_processor/importers/proto/proto_trace_reader.cc

LalitMaganti · 2026-02-13T17:59:57Z

src/trace_processor/importers/proto/protovm_incremental_tracing.cc

+TraceBlob MakeIncrementalStatePacket(
+    const protovm::Vm& vm,
+    const protos::pbzero::TracePacket::Decoder& patch) {
+  std::string incremental_state_without_trusted_fields =


This method needs to be rewritten to take as input a protozero::Message* to avoid a copy. Right now RwProto::SerializeAsString() is itself making two copies.

Apologies, I totally forgot I still needed to make this right. Happy to change the ProtoVM interface to fix it.

Done the refactoring on the ProtoVM side. Also available in the PR if you want to merge it separate:

#4816

src/trace_processor/importers/proto/protovm_incremental_tracing.cc

…ently This CL introduces a new class to write TracePacket protos efficiently and get out TraceBlobViews without creating tons of copies. There were already N instances of this in the codebase and we're now introducing a new one in #4768. Instead now we have a single class which works with TraceBlob + TraceBlobView and is stored in TraceProcessorContext so everyone can use it. It works in 4MB chunks and reuses them across TracePacket calls which makes it really efficient now to create these packets with 99% of cases being totally zero-copy.

…ently (#4799) This CL introduces a new class to write TracePacket protos efficiently and get out TraceBlobViews without creating tons of copies. There were already N instances of this in the codebase and we're now introducing a new one in #4768. Instead now we have a single class which works with TraceBlob + TraceBlobView and is stored in TraceProcessorContext so everyone can use it. It works in 4MB chunks and reuses them across TracePacket calls which makes it really efficient now to create these packets with 99% of cases being totally zero-copy.

LalitMaganti · 2026-02-16T16:10:16Z

Let's wait for #4816 to be merged and rebase

Change-Id: Ia5d69ed4f3f7314630c7a0217a0f4d3a43f9d1a8

rukkal · 2026-02-16T16:14:47Z

Let's wait for #4816 to be merged and rebase

Done

LalitMaganti · 2026-02-16T16:15:24Z

src/trace_processor/importers/proto/default_modules.cc

      new ChromeSystemProbesModule(module_context, context));
  module_context->modules.emplace_back(
      new MetadataMinimalModule(module_context, context));
+  module_context->modules.emplace_back(


This should be in additional_modules not here to prevent bloating Chromium binary size.

LalitMaganti · 2026-02-16T16:15:34Z

src/trace_processor/importers/proto/protovm_module.cc

+    : ProtoImporterModule(context), trace_context_(trace_context) {
+  RegisterForField(protos::pbzero::TracePacket::kTraceProvenanceFieldNumber);
+  RegisterForField(protos::pbzero::TracePacket::kProtovmsFieldNumber);
+  RegisterForField(


Why this? This feels like a signifcant layer violation.

If we need to work on this, the correct way to layer this is to have a protovm_tracker which is called by both protovm module and the winscope module.

This feels like a signifcant layer violation.

What do you mean? Are you expecting all the packets to be passed down to ProtoVmModule? Here my assumption was that you wanted to minimizet the "passing down"

a protovm_tracker which is called by both protovm module and the winscope module.

No idea what that protovm_tracker would be. Well, I guess it's something that gemini can decode for me :)

What do you mean? Are you expecting all the packets to be passed down to ProtoVmModule? Here my assumption was that you wanted to minimizet the "passing down"

I don't understand why the protovm module needs to know at all about surfaceflinger layer protos. That feels quite confusing no? What ties layers with protovm? Yes you happen to use ProtoVM for surfaceflinger layers but that's just how you happen to use it - nothing about protovm restricts it to happen for those packets.

The only code which should use or know about surfaceflinger protos is the winscope module.

No idea what that protovm_tracker would be. Well, I guess it's something that gemini can decode for me :)

Well you need some code which both the protovm module and the winscope module can talk to such that there's a "shared understanding" of how the protos should be modified. The general pattern for this in trace processor is by introducing a new shared "tracker" which both modules can talk to. Basically take all the stateful logic in this module and extract it into a separate class (as modules themselves cannot be referenced by other modules, that's a layering problem if they do).

That's correct, ProtoVmModule doesn't need to know anything about surfaceflinger or other winscope protos.

We could replace the

RegisterForField(protos::pbzero::TracePacket::kSurfaceflingerTransactionsFieldNumber);

with something lke

// just process any packet, including those that we already know the ProtoVMs don't care about RegisterForField(protos::pbzero::TracePacket::kTrustedPacketSequenceIdFieldNumber);

and stuff would work.

Shall we go with that "shared tracker" approach instead?

We could replace the

Yes we could but then every packet will end up in this code and this code becomes a lot more perf sensitive. Potentially at that state, we might need to add short-circuiting remove the module entirely and do this in ProtoTraceReader as before to avoid the virtual function call

However, right now I don't want to think in those terms because I'm very sure Chrome will start complaining about binary size if we start including code in core trace processor we don't need for JSON conversion. So I would prefer we go with the tracker approach. That keeps things flexible and localizes the perf impact to Winscope only.

- ProtoVmTracker encapsulates all the ProtoVM-related state - ProtoVmModule forwards to the tracker data from TraceProvenance and ProtoVms packets - Winscope module forwards to the tracker winscope packets that might be ProtoVM patches Change-Id: Ic47f37ad7e7cc84f64ff95c0f647d9857c1d07a7

LalitMaganti · 2026-02-17T13:21:54Z

src/trace_processor/types/trace_processor_context.h

  GlobalPtr<StackProfileTracker> stack_profile_tracker;
  GlobalPtr<Destructible> deobfuscation_tracker;  // DeobfuscationTracker
  GlobalPtr<BlobPacketWriter> blob_packet_writer;
+  GlobalPtr<Destructible> protovm_tracker;  // ProtoVmTracker


This should be PerTraceAndMachine not global.

LalitMaganti · 2026-02-17T13:22:37Z

src/trace_processor/importers/proto/BUILD.gn

    "proto_trace_reader.h",
    "proto_trace_tokenizer.cc",
    "proto_trace_tokenizer.h",
+    "protovm_module.cc",


This should be in the full target not minimal.

rukkal · 2026-02-17T13:25:31Z

src/trace_processor/importers/proto/protovm_tracker.cc

+  auto [data, size] = proto.SerializeAsUniquePtr();
+  auto blob = TraceBlob::TakeOwnership(std::move(data), size);
+
+  uint64_t timestamp;


@LalitMaganti I suspect you'll have questions about this. FYI here we read the timestamp from the serialized incremental state, so that we give a ProtoVM's program also the power to determine timestamps. Primiano wanted this feature so you might want to chat directly with him. Then we\ can always make the implementation below more efficient avoiding to construct a full TracePacket::Decoder.

The problem is that by doing this, your logic is now inconsistent with the protoTraceReader. I wasn't aware that you were planning on rewriting the full trace packet otherwise I wouldn't have told you to make a module at all.

In this case I actually think you have no choice but to do this very very proactively, basically at the very start of ProtoTraceReader, before any of the logic of that function. Otherwise, this code will drift between the (very complicted) timestamp logic which exists in [1]

[1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/perfetto/src/trace_processor/importers/proto/proto_trace_reader.cc

rukkal · 2026-02-17T13:27:49Z

src/trace_processor/importers/proto/winscope/winscope_module.cc

+          protovm_tracker_->TryProcessPatch(decoder, *packet,
+                                            std::move(sequence_state));
+      if (state) {
+        module_context_->trace_packet_stream->Push(state->timestamp,


@LalitMaganti any chance we can move the trace_packet_stream->Push() into ProtoVmTracker? A trace_packet_stream doesn't seem to be available from the tracker and not sure if exposing it breaks architectural boundaries

LalitMaganti · 2026-02-17T13:28:03Z

src/trace_processor/importers/proto/protovm_tracker.cc

+    const protos::pbzero::TracePacket::Decoder& patch,
+    const TraceBlobView& packet,
+    RefPtr<PacketSequenceStateGeneration> sequence_state) const {
+  protozero::HeapBuffered<protos::pbzero::TracePacket> proto;


Please use the new class I introduced for you - #4799

LalitMaganti · 2026-02-17T13:28:18Z

src/trace_processor/importers/proto/protovm_tracker.cc

+  for (int32_t producer_id : producer_ids) {
+    auto* sequence_ids = producer_id_to_sequence_ids_.Find(producer_id);
+    if (!sequence_ids) {
+      context_->storage->IncrementStats(stats::protovm_registration_error);


please use import log tracker rather than stats. Here and throughout.

LalitMaganti · 2026-02-17T13:31:25Z

src/trace_processor/importers/proto/protovm_tracker.cc

+  auto [data, size] = proto.SerializeAsUniquePtr();
+  auto blob = TraceBlob::TakeOwnership(std::move(data), size);
+
+  uint64_t timestamp;


The problem is that by doing this, your logic is now inconsistent with the protoTraceReader. I wasn't aware that you were planning on rewriting the full trace packet otherwise I wouldn't have told you to make a module at all.

In this case I actually think you have no choice but to do this very very proactively, basically at the very start of ProtoTraceReader, before any of the logic of that function. Otherwise, this code will drift between the (very complicted) timestamp logic which exists in [1]

[1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/perfetto/src/trace_processor/importers/proto/proto_trace_reader.cc

rukkal · 2026-02-17T16:51:05Z

src/trace_processor/importers/proto/proto_trace_reader.cc

+    // TODO(keanmariotti): fix this hack
+    decoder.protos::pbzero::TracePacket::Decoder::~Decoder();
+    new (&decoder)
+        protos::pbzero::TracePacket::Decoder(packet.data(), packet.length());


@LalitMaganti what about extending decoder's interface to fix this hack? Either an assignment operator or a Reset() member function

Change-Id: Ie48184c38ec9f461250f19bd6e87f031c9ee8c48

rukkal force-pushed the dev/rukkal/tp-protovm-integration branch 3 times, most recently from ca66a22 to 1edd49c Compare February 13, 2026 13:01

rukkal marked this pull request as ready for review February 13, 2026 13:31

rukkal requested a review from a team as a code owner February 13, 2026 13:31

rukkal requested a review from LalitMaganti February 13, 2026 13:35

LalitMaganti mentioned this pull request Feb 13, 2026

tp: introduce ForgedTracePacketWriter for writing TracePackets efficiently #4799

Merged

LalitMaganti reviewed Feb 13, 2026

View reviewed changes

rukkal force-pushed the dev/rukkal/tp-protovm-integration branch 2 times, most recently from ef645a4 to 735fcc9 Compare February 16, 2026 16:04

tp: integrate ProtoVM

b5d0889

Change-Id: Ia5d69ed4f3f7314630c7a0217a0f4d3a43f9d1a8

rukkal force-pushed the dev/rukkal/tp-protovm-integration branch from 735fcc9 to b5d0889 Compare February 16, 2026 16:13

LalitMaganti reviewed Feb 16, 2026

View reviewed changes

rukkal added 2 commits February 16, 2026 16:55

move ProtoVmModule into additional modules

7d50d94

LalitMaganti reviewed Feb 17, 2026

View reviewed changes

rukkal commented Feb 17, 2026

View reviewed changes

LalitMaganti reviewed Feb 17, 2026

View reviewed changes

rukkal commented Feb 17, 2026

View reviewed changes

rukkal force-pushed the dev/rukkal/tp-protovm-integration branch from bbea74f to e0e6d36 Compare February 17, 2026 16:53

revert to ProtoTraceReader + ProtoVmIncrementalTracing

8393379

Change-Id: Ie48184c38ec9f461250f19bd6e87f031c9ee8c48

rukkal force-pushed the dev/rukkal/tp-protovm-integration branch from e0e6d36 to 8393379 Compare February 17, 2026 16:57

Conversation

rukkal commented Feb 11, 2026

Uh oh!

github-actions bot commented Feb 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎨 Perfetto UI Builds

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LalitMaganti commented Feb 16, 2026

Uh oh!

rukkal commented Feb 16, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions bot commented Feb 11, 2026 •

edited

Loading