Skip to content

Display the event type from DOM events in chrome profiles #4604

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
julienw merged 3 commits into from
May 3, 2023
Merged

Display the event type from DOM events in chrome profiles #4604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d29be90
Display the event type from DOM events in chrome profiles
julienw May 2, 2023
3eb301b
Add a small test covering dom events
julienw May 2, 2023
efb3348
Display the marker timeline overview for imported profiles from Chrome
julienw Mar 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/components/timeline/TrackThread.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ class TimelineTrackThreadImpl extends PureComponent<Props> {
filteredThread.name === 'Compositor' ||
filteredThread.name === 'Renderer' ||
filteredThread.name === 'AndroidUI (JVM)' ||
filteredThread.name === 'CrRendererMain' ||
filteredThread.name === 'Merged thread' ||
filteredThread.name.startsWith('MediaDecoderStateMachine')) &&
processType !== 'plugin';
Expand Down
92 changes: 53 additions & 39 deletions src/profile-logic/import/chrome.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ function getThreadInfo(
// It looks like the TID information in Chrome's data isn't the system's TID
// but some internal values only unique for a pid. Therefore let's generate a
// proper unique value.
thread.tid = `${chunk.pid},${chunk.tid}`;
thread.tid = pidAndTid;

// Set the process type to something non-"Gecko". If this is left at
// "default", threads + processes without samples will not be auto-hidden in
Expand Down Expand Up @@ -861,6 +861,26 @@ function extractMarkers(
throw new Error('No "Other" category in empty profile category list');
}

profile.meta.markerSchema = [
{
name: 'EventDispatch',
chartLabel: '{marker.data.type2}',
tooltipLabel: '{marker.data.type2} - EventDispatch',
tableLabel: '{marker.data.type2}',
display: ['marker-chart', 'marker-table', 'timeline-overview'],
data: [
{
// In the original chrome profile, the key is `type`, but we rename it
// so that it doesn't clash with our internal `type` property.
key: 'type2',

@canova canova May 3, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about making this eventType instead?

@julienw julienw May 3, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that, but this would be right only for DispatchEvent markers. What if other markers also have type in their payload, but that's not related to events?

@canova canova May 3, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this schema's name is EventDispatch though. Do we expect other markers to have the same name as well?

@canova canova May 3, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see what you mean now. We need to change the code below that's generic. Hm, I guess it's okay to keep it this way.

@julienw julienw May 3, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add a comment to be more explicit about this though.

@canova canova May 3, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, a comment could be nice. It wasn't clear to me at first. Thanks!

label: 'Event Type',
format: 'string',
searchable: true,
},
],
},
];

for (const [name, events] of eventsByName.entries()) {
if (
name === 'Profile' ||
Expand Down Expand Up @@ -914,58 +934,52 @@ function extractMarkers(
}
markers.name.push(stringTable.indexForString(name));
markers.category.push(otherCategoryIndex);

if (argData && 'type' in argData) {
argData.type2 = argData.type;
}
if (argData && 'category' in argData) {
argData.category2 = argData.category;

@canova canova May 3, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we don't use the category2? Do we still need it? I may be missing something though.

@julienw julienw May 3, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that I don't want to lose any data.

You could argue that we can lose type2 and category2 😅

Maybe we can use something less typical such as __type or $type or __original__type?

@canova canova May 3, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I don't have a lot of good alternatives tbh. I think it's fine to keep type2 and catergory2 too then.

@julienw julienw May 3, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
It's super easy to change later if we need it too, as it's used only here with this indirection.

@julienw julienw May 3, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been thinking that our internal "type" should probably have been more collision-proof. But I guess this is a bit late for that now.

}

const newData = {
...argData,
type: name,
category: event.cat,
};

// $FlowExpectError Opt out of Flow checking for this one.
markers.data.push(newData);

if (event.ph === 'X') {
// Complete Event
// https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview#heading=h.lpfof2aylapb
const duration: number = (event.dur: any) / 1000;
markers.phase.push(INTERVAL);
markers.startTime.push(time);
markers.endTime.push(time + duration);

markers.data.push({
type: 'CompleteTraceEvent',
category: event.cat,
data: argData,
});
} else if (
event.ph === 'B' ||
event.ph === 'E' ||
event.ph === 'b' ||
event.ph === 'e'
) {
if (event.ph === 'B' || event.ph === 'b') {
// The 'B' and 'b' phases stand for "begin", and is the Chrome equivalent of IntervalStart.
markers.startTime.push(time);
markers.endTime.push(null);
markers.phase.push(INTERVAL_START);
} else {
// The 'E' and 'e' phase stand for "end", and is the Chrome equivalent of IntervalEnd.
markers.startTime.push(null);
markers.endTime.push(time);
markers.phase.push(INTERVAL_END);
}

// Duration or Async Event
} else if (event.ph === 'B' || event.ph === 'b') {
// Duration or Async Event Begin
// https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview#heading=h.nso4gcezn7n1
// The 'B' and 'b' phases stand for "begin", and is the Chrome equivalent of IntervalStart.
markers.startTime.push(time);
markers.endTime.push(null);
markers.phase.push(INTERVAL_START);
} else if (event.ph === 'E' || event.ph === 'e') {
// Duration or Async Event End
// https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview#heading=h.nso4gcezn7n1
markers.data.push({
type: 'tracing',
category: event.cat,
data: argData,
});
// The 'E' and 'e' phase stand for "end", and is the Chrome equivalent of IntervalEnd.
markers.startTime.push(null);
markers.endTime.push(time);
markers.phase.push(INTERVAL_END);
} else {
// Instant Event
// https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview#heading=h.lenwiilchoxp
// This assumes the phase is 'I' or 'i' (Instant), 'n' (Async Instant)
// or 'R' (Mark events)
markers.startTime.push(time);
markers.endTime.push(null);
markers.phase.push(INSTANT);

// Instant Event
// https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview#heading=h.lenwiilchoxp
markers.data.push({
type: 'InstantTraceEvent',
category: event.cat,
data: argData,
});
}
markers.length++;
}
Expand Down
Loading