Skip to content

Commit

Permalink
ui: cleanup Engine tracker
Browse files Browse the repository at this point in the history
Today there is a lot of unnecessary complexity around the logic that
counts the outstanding queries in the engine. Simplify that.
Also plumb engine explicitly through the sidebar and topbar rather
than depending on globals.
Also turns out we never had an explicit redraw scheduled after
each query response. This was happening by accident by virtue of
lazily updating the small box in the sidebar. Which also makes me
suspect we had a lingering bug where redraws-afte-query were not
scheduled properly if the sidebar was hidden.
This CL makes it explicit.

Change-Id: I3b739e13fa3ad5097d30d7c38182e9ed29ba1477
  • Loading branch information
primiano committed Oct 1, 2024
1 parent b05b609 commit 074514f
Show file tree
Hide file tree
Showing 11 changed files with 72 additions and 130 deletions.
47 changes: 0 additions & 47 deletions ui/src/controller/loading_manager.ts

This file was deleted.

11 changes: 4 additions & 7 deletions ui/src/controller/trace_controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ import {
} from '../trace_processor/wasm_engine_proxy';
import {showModal} from '../widgets/modal';
import {Controller} from './controller';
import {LoadingManager} from './loading_manager';
import {
TraceBufferStream,
TraceFileStream,
Expand All @@ -66,6 +65,7 @@ import {
import {ProfileType, profileType} from '../public/selection';
import {TraceInfo} from '../public/trace_info';
import {AppImpl} from '../core/app_trace_impl';
import {raf} from '../core/raf_scheduler';

type States = 'init' | 'loading_trace' | 'ready';

Expand Down Expand Up @@ -253,7 +253,7 @@ export class TraceController extends Controller<States> {
if (useRpc) {
console.log('Opening trace using native accelerator over HTTP+RPC');
engineMode = 'HTTP_RPC';
engine = new HttpRpcEngine(this.engineId, LoadingManager.getInstance);
engine = new HttpRpcEngine(this.engineId);
engine.errorHandler = (err) => {
globals.dispatch(
Actions.setEngineFailed({mode: 'HTTP_RPC', failure: `${err}`}),
Expand All @@ -264,18 +264,15 @@ export class TraceController extends Controller<States> {
console.log('Opening trace using built-in WASM engine');
engineMode = 'WASM';
const enginePort = resetEngineWorker();
engine = new WasmEngineProxy(
this.engineId,
enginePort,
LoadingManager.getInstance,
);
engine = new WasmEngineProxy(this.engineId, enginePort);
engine.resetTraceProcessor({
cropTrackEvents: CROP_TRACK_EVENTS_FLAG.get(),
ingestFtraceInRawTable: INGEST_FTRACE_IN_RAW_TABLE_FLAG.get(),
analyzeTraceProtoContent: ANALYZE_TRACE_PROTO_CONTENT_FLAG.get(),
ftraceDropUntilAllCpusValid: FTRACE_DROP_UNTIL_FLAG.get(),
});
}
engine.onResponseReceived = () => raf.scheduleFullRedraw();
this.engine = engine;

if (isMetatracingEnabled()) {
Expand Down
9 changes: 0 additions & 9 deletions ui/src/frontend/globals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ class Globals {
private _trackDataStore?: TrackDataStore = undefined;
private _overviewStore?: OverviewStore = undefined;
private _threadMap?: ThreadMap = undefined;
private _numQueriesQueued = 0;
private _bufferUsage?: number = undefined;
private _recordingLog?: string = undefined;
private _metricError?: string = undefined;
Expand Down Expand Up @@ -253,14 +252,6 @@ class Globals {
this._metricError = arg;
}

set numQueuedQueries(value: number) {
this._numQueriesQueued = value;
}

get numQueuedQueries() {
return this._numQueriesQueued;
}

get bufferUsage() {
return this._bufferUsage;
}
Expand Down
5 changes: 3 additions & 2 deletions ui/src/frontend/idle_detector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
// limitations under the License.

import {defer} from '../base/deferred';
import {globals} from './globals';
import {raf} from '../core/raf_scheduler';
import {AppImpl} from '../core/app_trace_impl';

/**
* This class is exposed by index.ts as window.waitForPerfettoIdle() and is used
Expand Down Expand Up @@ -68,8 +68,9 @@ export class IdleDetector {
}

private idleIndicators() {
const reqsPending = AppImpl.instance.trace?.engine.numRequestsPending ?? 0;
return [
globals.numQueuedQueries == 0,
reqsPending === 0,
!raf.hasPendingRedraws,
!document.getAnimations().some((a) => a.playState === 'running'),
document.querySelector('.progress.progress-anim') == null,
Expand Down
7 changes: 0 additions & 7 deletions ui/src/frontend/publish.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,6 @@ export function publishConversionJobStatusUpdate(
globals.publishRedraw();
}

export function publishLoading(numQueuedQueries: number) {
globals.numQueuedQueries = numQueuedQueries;
// TODO(hjd): Clean up loadingAnimation given that this now causes a full
// redraw anyways. Also this should probably just go via the global state.
raf.scheduleFullRedraw();
}

export function publishBufferUsage(args: {percentage: number}) {
globals.setBufferUsage(args.percentage);
globals.publishRedraw();
Expand Down
46 changes: 28 additions & 18 deletions ui/src/frontend/sidebar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import {openInOldUIWithSizeCheck} from './legacy_trace_viewer';
import {formatHotkey} from '../base/hotkeys';
import {SidebarMenuItem} from '../public/sidebar';
import {AppImpl} from '../core/app_trace_impl';
import {Trace} from '../public/trace';

const GITILES_URL =
'https://android.googlesource.com/platform/external/perfetto';
Expand Down Expand Up @@ -96,6 +97,10 @@ const VIZ_PAGE_IN_NAV_FLAG = featureFlags.register({
defaultValue: true,
});

export interface OptionalTraceAttrs {
trace?: Trace;
}

function shouldShowHiringBanner(): boolean {
return globals.isInternalUser && HIRING_BANNER_FLAG.get();
}
Expand Down Expand Up @@ -536,20 +541,20 @@ async function finaliseMetatrace(e: Event) {
downloadData('metatrace', result.metatrace, jsEvents);
}

const EngineRPCWidget: m.Component = {
view() {
class EngineRPCWidget implements m.ClassComponent<OptionalTraceAttrs> {
view({attrs}: m.CVnode<OptionalTraceAttrs>) {
let cssClass = '';
let title = 'Number of pending SQL queries';
let label: string;
let failed = false;
let mode: EngineMode | undefined;

const engine = globals.state.engine;
if (engine !== undefined) {
mode = engine.mode;
if (engine.failed !== undefined) {
const engineCfg = globals.state.engine;
if (engineCfg !== undefined) {
mode = engineCfg.mode;
if (engineCfg.failed !== undefined) {
cssClass += '.red';
title = 'Query engine crashed\n' + engine.failed;
title = 'Query engine crashed\n' + engineCfg.failed;
failed = true;
}
}
Expand Down Expand Up @@ -579,14 +584,15 @@ const EngineRPCWidget: m.Component = {
title += '\n(Query engine: built-in WASM)';
}

const numReqs = attrs.trace?.engine.numRequestsPending ?? 0;
return m(
`.dbg-info-square${cssClass}`,
{title},
m('div', label),
m('div', `${failed ? 'FAIL' : globals.numQueuedQueries}`),
m('div', `${failed ? 'FAIL' : numReqs}`),
);
},
};
}
}

const ServiceWorkerWidget: m.Component = {
view() {
Expand Down Expand Up @@ -668,11 +674,11 @@ const ServiceWorkerWidget: m.Component = {
},
};

const SidebarFooter: m.Component = {
view() {
class SidebarFooter implements m.ClassComponent<OptionalTraceAttrs> {
view({attrs}: m.CVnode<OptionalTraceAttrs>) {
return m(
'.sidebar-footer',
m(EngineRPCWidget),
m(EngineRPCWidget, attrs),
m(ServiceWorkerWidget),
m(
'.version',
Expand All @@ -687,8 +693,8 @@ const SidebarFooter: m.Component = {
),
),
);
},
};
}
}

class HiringBanner implements m.ClassComponent {
view() {
Expand All @@ -706,9 +712,9 @@ class HiringBanner implements m.ClassComponent {
}
}

export class Sidebar implements m.ClassComponent {
export class Sidebar implements m.ClassComponent<OptionalTraceAttrs> {
private _redrawWhileAnimating = new Animation(() => raf.scheduleFullRedraw());
view() {
view({attrs}: m.CVnode<OptionalTraceAttrs>) {
if (globals.hideSidebar) return null;
const vdomSections = [];
for (const section of getSections()) {
Expand Down Expand Up @@ -840,7 +846,11 @@ export class Sidebar implements m.ClassComponent {
),
m(
'.sidebar-scroll',
m('.sidebar-scroll-container', ...vdomSections, m(SidebarFooter)),
m(
'.sidebar-scroll-container',
...vdomSections,
m(SidebarFooter, attrs),
),
),
);
}
Expand Down
23 changes: 10 additions & 13 deletions ui/src/frontend/topbar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,20 +25,17 @@ import {Trace, TraceAttrs} from '../public/trace';

export const DISMISSED_PANNING_HINT_KEY = 'dismissedPanningHint';

class Progress implements m.ClassComponent {
view(_vnode: m.Vnode): m.Children {
const classes = classNames(this.isLoading() && 'progress-anim');
class Progress implements m.ClassComponent<TraceAttrs> {
view({attrs}: m.CVnode<TraceAttrs>): m.Children {
const engine = attrs.trace.engine;
const engineCfg = globals.getCurrentEngine();
const isLoading =
(engineCfg && !engineCfg.ready) ||
engine.numRequestsPending > 0 ||
taskTracker.hasPendingTasks();
const classes = classNames(isLoading && 'progress-anim');
return m('.progress', {class: classes});
}

private isLoading(): boolean {
const engine = globals.getCurrentEngine();
return (
(engine && !engine.ready) ||
globals.numQueuedQueries > 0 ||
taskTracker.hasPendingTasks()
);
}
}

class HelpPanningNotification implements m.ClassComponent {
Expand Down Expand Up @@ -136,7 +133,7 @@ export class Topbar implements m.ClassComponent<TopbarAttrs> {
'.topbar',
{class: globals.state.sidebarVisible ? '' : 'hide-sidebar'},
omnibox,
m(Progress),
attrs.trace && m(Progress, {trace: attrs.trace}),
m(HelpPanningNotification),
attrs.trace && m(TraceErrorIcon, {trace: attrs.trace}),
);
Expand Down
2 changes: 1 addition & 1 deletion ui/src/frontend/ui_main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -674,7 +674,7 @@ export class UiMainPerTrace implements m.ClassComponent {
{hotkeys},
m(
'main',
m(Sidebar),
m(Sidebar, {trace: this.trace}),
m(Topbar, {
omnibox: this.renderOmnibox(),
trace: this.trace,
Expand Down
Loading

0 comments on commit 074514f

Please sign in to comment.