Skip to content
Open
Changes from all commits
Commits
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
62 changes: 54 additions & 8 deletions ui/src/plugins/com.android.DayExplorer/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ export default class DayExplorerPlugin implements PerfettoPlugin {
trackName,
groupKey,
query,
childIter.track_id,
parentId === -1n, // isRoot
);
parent.addChildInOrder(node);
await this.addDayExplorerRecursive(ctx, node, limit, childIter.track_id);
Expand All @@ -116,6 +118,8 @@ export default class DayExplorerPlugin implements PerfettoPlugin {
name: string,
groupKey: string,
query: string,
trackId: bigint,
isRoot: boolean,
): Promise<TrackNode> {
const uri = `/day_explorer_${uuidv4()}`;
const renderer = await CounterTrack.createMaterialized({
Expand All @@ -130,6 +134,9 @@ export default class DayExplorerPlugin implements PerfettoPlugin {
renderer,
tags: {
kinds: [DAY_EXPLORER_TRACK_KIND],
trackId: trackId.toString(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this need to be a string? Does Perfetto not accept this being a number?

isLabelRoot: isRoot && name.startsWith('[Label]'),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is pretty tightly coupled with the google3 side. I think it would make more sense to encode this into the hierarchy. For example, it could be that every hierarchy root is a single root that avoids overlapping count. E.g.

- Regular (root 1)
  - Screen on
  - Screen off
- [Label] Media playback (root 2)
  - etc

isPhysicalRoot: isRoot && !name.startsWith('[Label]'),
},
});

Expand Down Expand Up @@ -178,27 +185,66 @@ export default class DayExplorerPlugin implements PerfettoPlugin {
currentSelection: AreaSelection,
): ReadonlyArray<QueryFlamegraphMetric> | undefined {
// The flame graph will be shown when any day explorer track is in the area
// selection. The selection is used to filter by time, but not by track. All
// day explorer tracks are considered for the graph.
let hasDayExplorer = false;
// selection. The selection is used to filter by time, and we filter the graph
// to only include energy from the selected tracks and their recursive descendants.
// If a physical track is selected, we exclude label roots to avoid double-counting.
const selectedTrackIds: bigint[] = [];
let hasPhysicalSelected = false;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is only looking for roots, so this can still overcount. For example:

[ ] - Screen on
[x]     - Compute
[x]     - Networking
[ ] - Screen off
[ ]     - Compute
[ ]     - Networking
[ ] - [Label] Media
[x]     - Compute
[ ]     - Networking

And this is probably worse even, since you lose the context of being under the label. Though, it's arguably a bad idea for someone to select like this :)

The original motivation for not filtering by track was to ensure all area selections had context. Additional filtering could be done on the flame chart.

I wonder if a better solution is to try to find the root for every track in the area selection. It would still allow overcounting, but only if you explicitly select tracks from regular and label tracks. And, if you do, you would at least have the appropriate context. But it would be better than today where it always shows the label tracks.


// First pass: identify if any physical roots are selected
for (const trackInfo of currentSelection.tracks) {
if (
trackInfo?.tags?.kinds?.includes(DAY_EXPLORER_TRACK_KIND) &&
trackInfo.tags.trackId !== undefined
) {
if (trackInfo.tags.isPhysicalRoot === true) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: why the additional nested if?

if (
  trackInfo?.tags?.kinds?.includes(DAY_EXPLORER_TRACK_KIND) &&
  trackInfo.tags.trackId !== undefined &&
  trackInfo.tags.isPhysicalRoot === true) {

hasPhysicalSelected = true;
}
}
}

// Second pass: collect selected IDs, filtering out label roots if physical tracks are selected
for (const trackInfo of currentSelection.tracks) {
if (trackInfo?.tags?.kinds?.includes(DAY_EXPLORER_TRACK_KIND)) {
hasDayExplorer = true;
break;
if (
trackInfo?.tags?.kinds?.includes(DAY_EXPLORER_TRACK_KIND) &&
trackInfo.tags.trackId !== undefined
) {
const isLabelRoot = trackInfo.tags.isLabelRoot === true;
if (hasPhysicalSelected && isLabelRoot) {
// Exclude label roots if physical tracks are selected to avoid double-counting
continue;
}
const trackId = trackInfo.tags.trackId;
if (typeof trackId === 'string' || typeof trackId === 'number') {
selectedTrackIds.push(BigInt(trackId));
}
}
}
if (!hasDayExplorer) {

if (selectedTrackIds.length === 0) {
return undefined;
}

const metrics = metricsFromTableOrSubquery({
tableOrSubquery: `
(
WITH
WITH RECURSIVE
selected_roots(track_id) AS (
SELECT column1 FROM (VALUES ${selectedTrackIds.map((id) => `(${id})`).join(',')})
),
descendants(track_id) AS (
SELECT track_id FROM selected_roots
UNION ALL
SELECT child.track_id
FROM day_explorer_ui_hierarchy child
JOIN descendants parent ON child.parent_id = parent.track_id
),
total_energy AS (
SELECT track_id, parent_id, display_name, SUM(energy_uws) AS energy_uws
FROM day_explorer_ui_hierarchy_per_ts
WHERE ts >= ${currentSelection.start}
AND ts <= ${currentSelection.end}
AND track_id IN (SELECT track_id FROM descendants)
GROUP BY 1, 2, 3
),
with_child AS (
Expand Down
Loading