base: Fix trace events with TRACE_EVENT_PHASE_COMPLETE [chromium/src : main]
0 views
Skip to first unread message
Sunny Sachanandani (Gerrit)
Jun 6, 2024, 6:52:15 PMJun 6
to Eric Seckler, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Attention needed from Eric Seckler
Sunny Sachanandani voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sunny Sachanandani
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Eric Seckler
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eric Seckler (Gerrit)
Jun 7, 2024, 4:50:22 AMJun 7
to Sunny Sachanandani, Mikhail Khokhlov, Chromium LUCI CQ, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Attention needed from Mikhail Khokhlov and Sunny Sachanandani
Eric Seckler voted and added 1 comment![]( "Open in Gerrit")
Votes added by Eric Seckler
| Code-Review | +1 |
1 comment
Patchset-level comments
File-level comment, Patchset 3 (Latest):
Eric Seckler . resolved
+Mikhail for review as well, but I think this should be fine.
Related details
Attention is currently required from:
- Mikhail Khokhlov
- Sunny Sachanandani
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eric Seckler (Gerrit)
Jun 7, 2024, 4:54:15 AMJun 7
to Sunny Sachanandani, Mikhail Khokhlov, Chromium LUCI CQ, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Attention needed from Mikhail Khokhlov and Sunny Sachanandani
Eric Seckler added 1 comment![]( "Open in Gerrit")
File base/trace_event/trace_log.cc
Line 267, Patchset 3 (Latest):
auto phase = trace_event->phase();Eric Seckler . unresolved
Actually minor nit: Can we move this `phase` local outside of the write_args lambda?
Looks like it (and the logic to override it to PHASE_BEGIN for PHASE_COMPLETE) already exists a few lines down, so all we need to do is move it above the lambda and capture it into it?
Related details
Attention is currently required from:
- Mikhail Khokhlov
- Sunny Sachanandani
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikhail Khokhlov (Gerrit)
Jun 7, 2024, 5:44:17 AMJun 7
to Sunny Sachanandani, Eric Seckler, Chromium LUCI CQ, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Attention needed from Sunny Sachanandani
Mikhail Khokhlov voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Sunny Sachanandani
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sunny Sachanandani (Gerrit)
Jun 7, 2024, 11:53:28 AMJun 7
to Mikhail Khokhlov, Eric Seckler, Chromium LUCI CQ, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Sunny Sachanandani voted and added 2 comments![]( "Open in Gerrit")
Votes added by Sunny Sachanandani
| Auto-Submit | +1 |
| Commit-Queue | +2 |
2 comments
Patchset-level comments
File base/trace_event/trace_log.cc
Actually minor nit: Can we move this `phase` local outside of the write_args lambda?
Looks like it (and the logic to override it to PHASE_BEGIN for PHASE_COMPLETE) already exists a few lines down, so all we need to do is move it above the lambda and capture it into it?
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sunny Sachanandani (Gerrit)
Jun 7, 2024, 12:19:48 PMJun 7
to Mikhail Khokhlov, Eric Seckler, Chromium LUCI CQ, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Sunny Sachanandani voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Jun 7, 2024, 1:33:22 PMJun 7
to Sunny Sachanandani, Mikhail Khokhlov, Eric Seckler, chromium...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
3 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: base/trace_event/trace_log.cc
Insertions: 7, Deletions: 9.
@@ -259,15 +259,16 @@
TraceLog::GetInstance()->GetCategoryGroupName(
trace_event->category_group_enabled()));
- auto write_args = [trace_event](perfetto::EventContext ctx) {
+ auto phase = trace_event->phase();
+ if (phase == TRACE_EVENT_PHASE_COMPLETE) {
+ phase = TRACE_EVENT_PHASE_BEGIN;
+ }
+
+ auto write_args = [trace_event, phase](perfetto::EventContext ctx) {
WriteDebugAnnotations(trace_event, ctx.event());
uint32_t id_flags = trace_event->flags() & (TRACE_EVENT_FLAG_HAS_ID |
TRACE_EVENT_FLAG_HAS_LOCAL_ID |
TRACE_EVENT_FLAG_HAS_GLOBAL_ID);
- auto phase = trace_event->phase();
- if (phase == TRACE_EVENT_PHASE_COMPLETE) {
- phase = TRACE_EVENT_PHASE_BEGIN;
- }
if (!id_flags &&
perfetto::internal::TrackEventLegacy::PhaseToType(phase) !=
perfetto::protos::pbzero::TrackEvent::TYPE_UNSPECIFIED) {
@@ -290,14 +291,11 @@
}
};
- auto phase = trace_event->phase();
auto flags = trace_event->flags();
base::TimeTicks timestamp = trace_event->timestamp().is_null()
? TRACE_TIME_TICKS_NOW()
: trace_event->timestamp();
- if (phase == TRACE_EVENT_PHASE_COMPLETE) {
- phase = TRACE_EVENT_PHASE_BEGIN;
- } else if (phase == TRACE_EVENT_PHASE_INSTANT) {
+ if (phase == TRACE_EVENT_PHASE_INSTANT) {
auto scope = flags & TRACE_EVENT_FLAG_SCOPE_MASK;
switch (scope) {
case TRACE_EVENT_SCOPE_GLOBAL:
```
Change information
Commit message:
base: Fix trace events with TRACE_EVENT_PHASE_COMPLETE
Skia uses TRACE_EVENT_PHASE_COMPLETE and UpdateTraceEventDuration() for
scoped trace events. This isn't working correctly - the traces show up
with zero duration and can mess up subsequent non-Skia trace events too.
This can be traced to a bug in OnAddLegacyTraceEvent() - it first tries
to patch TRACE_EVENT_PHASE_COMPLETE to TRACE_EVENT_PHASE_BEGIN so that
perfetto can record the event, but later it annotates the event with
`set_legacy_event` if the original phase doesn't have a perfetto
equivalent type which is the case with TRACE_EVENT_PHASE_COMPLETE.
This CL fixes that bug by overriding TRACE_EVENT_PHASE_COMPLETE to
TRACE_EVENT_PHASE_BEGIN before checking for annotating the event as a
legacy event. This ensures the code works like before for other kinds
of non-perfetto phase types like async begin/end, arg metadata, etc.
Bug: 345367404
Change-Id: If0c42cecaa857e62f2c9c80c3ce7aa2c450b189b
Auto-Submit: Sunny Sachanandani <sun...@chromium.org>
Commit-Queue: Sunny Sachanandani <sun...@chromium.org>
Reviewed-by: Eric Seckler <esec...@chromium.org>
Reviewed-by: Mikhail Khokhlov <khok...@google.com>
Cr-Commit-Position: refs/heads/main@{#1312103}
Files:
- M base/trace_event/trace_log.cc
Change size: S
Delta: 1 file changed, 10 insertions(+), 8 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Mikhail Khokhlov, +1 by Eric Seckler
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages