Jesse McKenna (Gerrit)

unread,

Jun 16, 2026, 10:56:00 PMJun 16

to Etienne Pierre-Doray, David Bienvenu, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Chromium LUCI CQ, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Etienne Pierre-Doray

Jesse McKenna voted and added 1 comment

Votes added by Jesse McKenna

Commit-Queue

+1

1 comment

Patchset-level comments

File-level comment, Patchset 3 (Latest):

Jesse McKenna . resolved

PTAL, thank you!

Open in Gerrit

Related details

Attention is currently required from:

Etienne Pierre-Doray

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Etienne Pierre-Doray (Gerrit)

unread,

Jun 17, 2026, 1:50:16 PMJun 17

to Jesse McKenna, David Bienvenu, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Chromium LUCI CQ, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Jesse McKenna

Etienne Pierre-Doray added 1 comment

File components/tracing/common/etw_consumer_win.cc

Line 134, Patchset 3 (Latest):

  auto trace_packet = trace_writer_->NewTracePacket();
  trace_packet->set_sequence_flags(
      perfetto::protos::pbzero::TracePacket::SEQ_INCREMENTAL_STATE_CLEARED);

Etienne Pierre-Doray . unresolved

Can we reuse this packet in EtwConsumer::StartNewPacket instead of creating a new one? (maybe this can be achieved simply by early exit below)

It seems like ResetEmittedState() can also be called from HandleStackWalkEvent, in which case we don't set top level timestamp.

Open in Gerrit

Related details

Attention is currently required from:

Jesse McKenna

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Jesse McKenna (Gerrit)

unread,

Jun 17, 2026, 9:21:08 PMJun 17

to Etienne Pierre-Doray, David Bienvenu, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Chromium LUCI CQ, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Etienne Pierre-Doray

Jesse McKenna added 1 comment

File components/tracing/common/etw_consumer_win.cc

Line 134, Patchset 3: auto trace_packet = trace_writer_->NewTracePacket();


  trace_packet->set_sequence_flags(
      perfetto::protos::pbzero::TracePacket::SEQ_INCREMENTAL_STATE_CLEARED);

Etienne Pierre-Doray . resolved

Can we reuse this packet in EtwConsumer::StartNewPacket instead of creating a new one? (maybe this can be achieved simply by early exit below)
It seems like ResetEmittedState() can also be called from HandleStackWalkEvent, in which case we don't set top level timestamp.

Jesse McKenna

Done - thanks for the guidance, fixing this made it a lot simpler and also fixed the trybots : ) Now `NewTracePacket` is only called from `StartNewPacket`, which makes a lot more sense.

Please feel free to let me know if anything else seems off.

Open in Gerrit

Related details

Attention is currently required from:

Etienne Pierre-Doray

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Etienne Pierre-Doray (Gerrit)

unread,

Jun 18, 2026, 8:27:56 AMJun 18

to Jesse McKenna, David Bienvenu, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Chromium LUCI CQ, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Jesse McKenna

Etienne Pierre-Doray added 1 comment

File components/tracing/common/etw_consumer_win.cc

Line 444, Patchset 4 (Latest): ResetEmittedState();

Etienne Pierre-Doray . unresolved

This will clear `reset_emitted_state_`, which means the following call to StartNewPacket (below `interned_callstack.was_emitted` will always be false following ResetEmittedState) would incorrectly use SEQ_NEEDS_INCREMENTAL_STATE?

Maybe we make ResetEmittedState() *not* update reset_emitted_state_ (and only clear the cache), and we do in StartNewPacket something like:

```
if (reset_emitted_state_.exchange(false, std::memory_order_relaxed)) {
  ResetEmittedState();
  sequence_flags =
    perfetto::protos::pbzero::TracePacket::SEQ_INCREMENTAL_STATE_CLEARED;
}
```

Open in Gerrit

Related details

Attention is currently required from:

Jesse McKenna

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Jesse McKenna (Gerrit)

unread,

Jun 18, 2026, 2:15:35 PMJun 18

to Etienne Pierre-Doray, David Bienvenu, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Chromium LUCI CQ, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Etienne Pierre-Doray

Jesse McKenna added 1 comment

File components/tracing/common/etw_consumer_win.cc

Line 444, Patchset 4: ResetEmittedState();

Etienne Pierre-Doray . resolved

This will clear `reset_emitted_state_`, which means the following call to StartNewPacket (below `interned_callstack.was_emitted` will always be false following ResetEmittedState) would incorrectly use SEQ_NEEDS_INCREMENTAL_STATE?
Maybe we make ResetEmittedState() *not* update reset_emitted_state_ (and only clear the cache), and we do in StartNewPacket something like:
```
if (reset_emitted_state_.exchange(false, std::memory_order_relaxed)) {
  ResetEmittedState();
  sequence_flags =
    perfetto::protos::pbzero::TracePacket::SEQ_INCREMENTAL_STATE_CLEARED;
}
```

Jesse McKenna

That makes sense, thanks! Done

Open in Gerrit

Related details

Attention is currently required from:

Etienne Pierre-Doray

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Etienne Pierre-Doray (Gerrit)

unread,

Jun 18, 2026, 3:51:18 PMJun 18

to Jesse McKenna, David Bienvenu, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Chromium LUCI CQ, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Attention needed from Jesse McKenna

Etienne Pierre-Doray voted and added 1 comment

Votes added by Etienne Pierre-Doray

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Etienne Pierre-Doray . resolved

LGTM, thanks!

Open in Gerrit

Related details

Attention is currently required from:

Jesse McKenna

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Jesse McKenna (Gerrit)

unread,

Jun 18, 2026, 6:49:15 PMJun 18

to Etienne Pierre-Doray, David Bienvenu, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, Chromium LUCI CQ, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Jesse McKenna voted and added 1 comment

Votes added by Jesse McKenna

Commit-Queue

+2

1 comment

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Jesse McKenna . resolved

Thanks for the thorough review!

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Jun 18, 2026, 6:55:01 PMJun 18

to Jesse McKenna, Etienne Pierre-Doray, David Bienvenu, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

etw-stackwalk: clear initial incremental state

https://crrev.com/c/7794326 added flags SEQ_NEEDS_INCREMENTAL_STATE and
SEQ_INCREMENTAL_STATE_CLEARED to trace packets emitted by EtwConsumer.
However, that seems to cause an error when opening traces with
EtwConsumer events [1].

According to trace_packet.proto, packets with the
SEQ_NEEDS_INCREMENTAL_STATE flag must be preceded by packets with the
SEQ_INCREMENTAL_STATE_CLEARED flag. This change ensures that the first
trace packet and those after incremental state is reset are preceded by
a packet with the SEQ_INCREMENTAL_STATE_CLEARED flag.

[1] Error:
traced_buf_incremental_sequences_dropped
For a given buffer, indicates the number of sequences where all the
packets on that sequence were dropped due to lack of a valid incremental
state (i.e. interned data). This is usually a strong sign that either:
1) incremental state invalidation is disabled. 2) the incremental state
invalidation interval is too low. In either case, see
https://perfetto.dev/docs/concepts/buffers#incremental-state-in-trace-packets

Bug: 400769265

Change-Id: I39902731820363cf80740b57e350b3a50856242d

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7953183

Commit-Queue: Jesse McKenna <jessem...@google.com>

Reviewed-by: Etienne Pierre-Doray <etie...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1649367}

Files:

M components/tracing/common/etw_consumer_win.cc
M components/tracing/common/etw_consumer_win.h

Change size: S

Delta: 2 files changed, 9 insertions(+), 9 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Etienne Pierre-Doray

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

etw-stackwalk: clear initial incremental state [chromium/src : main]

Jesse McKenna (Gerrit)

Jesse McKenna voted and added 1 comment

Votes added by Jesse McKenna

1 comment

Related details

Etienne Pierre-Doray (Gerrit)

Etienne Pierre-Doray added 1 comment

Related details

Jesse McKenna (Gerrit)

Jesse McKenna added 1 comment

Related details

Etienne Pierre-Doray (Gerrit)

Etienne Pierre-Doray added 1 comment

Related details

Jesse McKenna (Gerrit)

Jesse McKenna added 1 comment

Related details

Etienne Pierre-Doray (Gerrit)

Etienne Pierre-Doray voted and added 1 comment

Votes added by Etienne Pierre-Doray

1 comment

Related details

Jesse McKenna (Gerrit)

Jesse McKenna voted and added 1 comment

Votes added by Jesse McKenna

1 comment

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information