Skip to content

Commit 8828ddd

Browse files
and-oliDevtools-frontend LUCI CQ
authored andcommitted
[Perf panel] Prefer profile calls over trace events when sorting
If timestamp and duration are equal between a trace event and a profile call, the profile call should be put first when sorting in ascending order. This is because when the timesamp of a profile call is exactly the same as a trace event's, it's likely because the SamplesIntegrator sets the profile call's ts so that it ends up parenting the trace event in the flame chart. Without this extra check in the comparator, the precedence meant by the samples integrator isn't certain to happen. Bug: 389056697 Change-Id: I8c3754782397ab640888ee3fa5fa3e91772e6aef Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6264603 Reviewed-by: Alina Varkki <[email protected]> Commit-Queue: Andres Olivares <[email protected]>
1 parent 931c509 commit 8828ddd

File tree

4 files changed

+14
-5
lines changed

4 files changed

+14
-5
lines changed

front_end/models/trace/helpers/SamplesIntegrator.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ describeWithEnvironment('SamplesIntegrator', function() {
106106
throw new Error('Trace events were unexpectedly not found.');
107107
}
108108
const constructedCalls = samplesIntegrator.buildProfileCalls(traceEvents);
109-
assert.lengthOf(constructedCalls, 5131);
109+
assert.lengthOf(constructedCalls, 5300);
110110
});
111111

112112
it('creates JS profile calls with a top-level V8 invocation', () => {

front_end/models/trace/helpers/Trace.ts

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ export interface TimeSpan {
107107
ts: Types.Timing.Micro;
108108
dur?: Types.Timing.Micro;
109109
}
110-
export function eventTimeComparator(a: TimeSpan, b: TimeSpan): -1|0|1 {
110+
export function eventTimeComparator(a: Types.Events.Event, b: Types.Events.Event): -1|0|1 {
111111
const aBeginTime = a.ts;
112112
const bBeginTime = b.ts;
113113
if (aBeginTime < bBeginTime) {
@@ -126,13 +126,23 @@ export function eventTimeComparator(a: TimeSpan, b: TimeSpan): -1|0|1 {
126126
if (aEndTime < bEndTime) {
127127
return 1;
128128
}
129+
// If times are equal, prioritize profile calls over trace events,
130+
// since an exactly equal timestamp with a trace event is likely
131+
// indicates that the SamplesIntegrator meant to parent the trace
132+
// event with the profile call.
133+
if (Types.Events.isProfileCall(a)) {
134+
return -1;
135+
}
136+
if (Types.Events.isProfileCall(b)) {
137+
return 1;
138+
}
129139
return 0;
130140
}
131141
/**
132142
* Sorts all the events in place, in order, by their start time. If they have
133143
* the same start time, orders them by longest first.
134144
*/
135-
export function sortTraceEventsInPlace(events: Array<{ts: Types.Timing.Micro, dur?: Types.Timing.Micro}>): void {
145+
export function sortTraceEventsInPlace(events: Types.Events.Event[]): void {
136146
events.sort(eventTimeComparator);
137147
}
138148

front_end/panels/timeline/TimelineTreeView.test.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -296,7 +296,6 @@ describeWithEnvironment('TimelineTreeView', function() {
296296
'https://www.googletagmanager.com/gtag/js?id=G-18JR3Q8PJ8&l=dataLayer&cx=c',
297297
'https://www.google-analytics.com/j/collect?v=1&_v=j101&a=68725886&t=event&ni=1&_s=1&dl=https%3A%2F%2Fweb.dev%2F&ul=en-gb&de=UTF-8&dt=web.dev&sd=24-bit&sr=3360x1890&vp=1665x846&je=0&ec=Web%20Vitals&ea=FCP&el=v3-1696581005645-6472407333688&ev=129&_u=QACAAEABAAAAACAAIg~&jid=&gjid=&cid=1874137241.1685438100&tid=UA-126406676-2&_gid=656288571.1696581004&_slc=1&gtm=45He3a40n81MZWCJPP&cd5=15&cd6=navigate&cd7=light&cd8=dom-content-loaded&cd9=8&z=54974500',
298298
'https://shared-storage-demo-content-producer.web.app/paa/scripts/private-aggregation-test.js',
299-
'https://shared-storage-demo-content-producer.web.app/paa/scripts/private-aggregation-test.html',
300299
'https://web-dev.imgix.net/image/SZHNhsfjU9RbCestTGZU6N7JEWs1/VwL892KEz6bakZMlq10D.png?auto=format&w=740',
301300
]);
302301
});

front_end/panels/timeline/utils/AICallTree.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,13 +144,13 @@ Children:
144144
* 4 – _ds.q.ns
145145
146146
Node: 4 – _ds.q.ns
147-
Selected: true
148147
dur: 0.2
149148
URL #: 0
150149
Children:
151150
* 5 – clearTimeout
152151
153152
Node: 5 – clearTimeout
153+
Selected: true
154154
dur: 0.2
155155
self: 0
156156
Children:

0 commit comments

Comments
 (0)