Skip to content

Commit

Permalink
ui: Fix UiMain lifecycle
Browse files Browse the repository at this point in the history 
Today UiMain has an onremove and a disposable stack, but
today (before this CL) those are pure placebo. In fact,
today UiMain is never destroyed.
This CL fixes that by wrapping it in a component that takes
care of creating a new UiMain instance for each new trace.
It does so by carefully linearizing with the point where
globals is initialized, so that everybody else sees globals'
entries in a consistent state.
This is required because we register Notes editor and Details
tabs in UiMain (which itself is okay). Until now we had only
one instance forever for those tabs, and it was fine because
their state was living in globals.state, which got recycled on
every trace. Now we are moving to a world where each manager
holds its own state, which means registering a tab only once
won't be enough.
In general feels nice to tie the lifecycle of the UiMain
component to a trace lifetime, as today it does register a
bunch of commands that need to be unregistered and re-registered
on each trace.
Also remove the overview track from globals and put it into
tickmark panel, which is the only user of it.

Change-Id: If8c9776aab5635b0a3d2ec917d2fe619e376ae9b
  • Loading branch information
@primiano
primiano committed Sep 23, 2024
1 parent b96e6f6 commit 2fadc23
Show file tree
Hide file tree
Showing 6 changed files with 121 additions and 100 deletions.
8 changes: 8 additions & 0 deletions ui/src/base/disposable_stack.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,15 @@ export class AsyncDisposableStack implements AsyncDisposable {
if (res === undefined) {
break;
}
const timerId = setTimeout(() => {
throw new Error(
'asyncDispose timed out. This might be due to a Disposable ' +
'resource trying to issue cleanup queries on trace unload, ' +
'while the Wasm module was already destroyed ',
);
}, 10_000);
await res[Symbol.asyncDispose]();
clearTimeout(timerId);
}
}

Expand Down
38 changes: 8 additions & 30 deletions ui/src/frontend/globals.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,6 @@ import {exists} from '../base/utils';
import {OmniboxManagerImpl} from '../core/omnibox_manager';
import {SerializedAppState} from '../common/state_serialization_schema';
import {getServingRoot} from '../base/http_utils';
import {
createSearchOverviewTrack,
SearchOverviewTrack,
} from './search_overview_track';
import {TraceInfo} from '../public/trace_info';
import {Registry} from '../base/registry';
import {SidebarMenuItem} from '../public/sidebar';
Expand Down Expand Up @@ -205,8 +201,8 @@ class Globals {
private _selectionManager = new SelectionManagerImpl();
private _noteManager = new NoteManagerImpl();
private _hasFtrace: boolean = false;
private _searchOverviewTrack?: SearchOverviewTrack;
private _workspaceManager = new WorkspaceManagerImpl();
private _currentTraceId = '';
readonly omnibox = new OmniboxManagerImpl();

httpRpcState: HttpRpcState = {connected: false};
Expand Down Expand Up @@ -286,29 +282,11 @@ class Globals {
},
});

// TODO(stevegolton): Even though createSearchOverviewTrack() returns a
// disposable, we completely ignore it as we assume the dispose action
// includes just dropping some tables, and seeing as this object will live
// for the duration of the trace/engine, there's no need to drop anything as
// the tables will be dropped along with the trace anyway.
//
// Note that this is no worse than a lot of the rest of the app where tables
// are created with no way to drop them.
//
// Once we fix the story around loading new traces, we should tidy this up.
// We could for example have a matching globals.onTraceUnload() that
// performs any tear-down before the old engine is dropped. This might seem
// pointless, but it could at least block until any currently running update
// cycles complete, to avoid leaving promises open on old engines that will
// never resolve.
//
// Alternatively we could decide that we don't want to support switching
// traces at all, in which case we can ignore tear down entirely.
this._searchOverviewTrack = await createSearchOverviewTrack(
engine,
this.searchManager,
this.timeline,
);
// Incrementing this causes the root mithril component UiMain to be
// recreated. This is to linearize globals and UiMain and guarrantee that by
// the time UiMain gets created, it can consistently see the globals for the
// new trace.
this._currentTraceId = engine.engineId;
}

// Used for permalink load by trace_controller.ts.
Expand Down Expand Up @@ -488,8 +466,8 @@ class Globals {
return this._hasFtrace;
}

get searchOverviewTrack() {
return this._searchOverviewTrack;
get currentTraceId() {
return this._currentTraceId;
}

getConversionJobStatus(name: ConversionJobName): ConversionJobStatus {
Expand Down
50 changes: 0 additions & 50 deletions ui/src/frontend/notes.ts
View file

This file was deleted.

44 changes: 41 additions & 3 deletions ui/src/frontend/tickmark_panel.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,50 @@ import {Size2D} from '../base/geom';
import {Panel} from './panel_container';
import {TimeScale} from '../base/time_scale';
import {canvasClip} from '../base/canvas_utils';
import {
createSearchOverviewTrack,
SearchOverviewTrack,
} from './search_overview_track';
import {getEngine} from './get_engine';
import {Optional} from '../base/utils';

// TODO(primiano): this should be moved into the TraceImpl object rather than
// being a global.
let lastOverviewTrack: Optional<{
readonly engineId: string;
readonly track: SearchOverviewTrack;
}> = undefined;

// This is used to display the summary of search results.
export class TickmarkPanel implements Panel {
readonly kind = 'panel';
readonly selectable = false;
private searchOverviewTrack?: SearchOverviewTrack;

constructor() {
// We want to create the overview track only once per trace, but this
// class can be delete and re-instantiated when switching between pages via
// the sidebar. So we cache globally the overview track, and re-instantiate
// it if the trace changes.
const curEngineId = globals.getCurrentEngine()?.id;
if (curEngineId === undefined) return;
if (lastOverviewTrack && curEngineId === lastOverviewTrack?.engineId) {
this.searchOverviewTrack = lastOverviewTrack.track;
} else {
// NOTE: SearchOverviewTrack is Disposable but we don't add it to the
// trash, as its dispose() tries to clean up tables in TraceProcessor.
// Doing so is useless and harmful, for the reasons described in the
// comment UiMain::onbeforeremove().
createSearchOverviewTrack(
getEngine('SearchOverviewTrack'),
globals.searchManager,
globals.timeline,
).then((track) => {
lastOverviewTrack = {engineId: curEngineId, track};
this.searchOverviewTrack = track;
});
}
}

render(): m.Children {
return m('.tickbar');
Expand Down Expand Up @@ -63,9 +102,8 @@ export class TickmarkPanel implements Panel {
}
}

const searchOverviewRenderer = globals.searchOverviewTrack;
if (searchOverviewRenderer) {
searchOverviewRenderer.render(ctx, size);
if (this.searchOverviewTrack) {
this.searchOverviewTrack.render(ctx, size);
}
}
}
75 changes: 58 additions & 17 deletions ui/src/frontend/ui_main.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ import {onClickCopy} from './clipboard';
import {CookieConsent} from './cookie_consent';
import {getTimeSpanOfSelectionOrVisibleWindow, globals} from './globals';
import {toggleHelp} from './help_modal';
import {Notes} from './notes';
import {Omnibox, OmniboxOption} from './omnibox';
import {addQueryResultsTab} from './query_result_tab';
import {Sidebar} from './sidebar';
Expand All @@ -47,6 +46,10 @@ import {OmniboxMode} from '../core/omnibox_manager';
import {PromptOption} from '../public/omnibox';
import {DisposableStack} from '../base/disposable_stack';
import {Spinner} from '../widgets/spinner';
import {NotesEditorTab} from './notes_panel';
import {NotesListEditor} from './notes_list_editor';

const OMNIBOX_INPUT_REF = 'omnibox';

function renderPermalink(): m.Children {
const hash = globals.permalinkHash;
Expand All @@ -72,17 +75,24 @@ class Alerts implements m.ClassComponent {
}
}

// This wrapper creates a new instance of UiMainPerTrace for each new trace
// loaded (including the case of no trace at the beginning).
export class UiMain implements m.ClassComponent {
view({children}: m.CVnode) {
return [m(UiMainPerTrace, {key: globals.currentTraceId}, children)];
}
}

// This components gets destroyed and recreated every time the current trace
// changes. Note that in the beginning the current trace is undefined.
export class UiMainPerTrace implements m.ClassComponent {
// NOTE: this should NOT need to be an AsyncDisposableStack. If you feel the
// need of making it async because you want to clean up SQL resources, that
// will cause bugs (see comments in oncreate()).
private trash = new DisposableStack();
static readonly OMNIBOX_INPUT_REF = 'omnibox';
private omniboxInputEl?: HTMLInputElement;
private recentCommands: string[] = [];

constructor() {
this.trash.use(new Notes());
this.trash.use(new AggregationsTabs());
}

private cmds: Command[] = [
{
id: 'perfetto.SetTimestampFormat',
Expand Down Expand Up @@ -367,7 +377,7 @@ export class UiMain implements m.ClassComponent {
return m(Omnibox, {
value: globals.omnibox.text,
placeholder: prompt.text,
inputRef: UiMain.OMNIBOX_INPUT_REF,
inputRef: OMNIBOX_INPUT_REF,
extraClasses: 'prompt-mode',
closeOnOutsideClick: true,
options,
Expand Down Expand Up @@ -427,7 +437,7 @@ export class UiMain implements m.ClassComponent {
return m(Omnibox, {
value: globals.omnibox.text,
placeholder: 'Filter commands...',
inputRef: UiMain.OMNIBOX_INPUT_REF,
inputRef: OMNIBOX_INPUT_REF,
extraClasses: 'command-mode',
options,
closeOnSubmit: true,
Expand Down Expand Up @@ -471,7 +481,7 @@ export class UiMain implements m.ClassComponent {
return m(Omnibox, {
value: globals.omnibox.text,
placeholder: ph,
inputRef: UiMain.OMNIBOX_INPUT_REF,
inputRef: OMNIBOX_INPUT_REF,
extraClasses: 'query-mode',

onInput: (value) => {
Expand Down Expand Up @@ -504,7 +514,7 @@ export class UiMain implements m.ClassComponent {
return m(Omnibox, {
value: globals.omnibox.text,
placeholder: "Search or type '>' for commands or ':' for SQL mode",
inputRef: UiMain.OMNIBOX_INPUT_REF,
inputRef: OMNIBOX_INPUT_REF,
onInput: (value, _prev) => {
if (value === '>') {
globals.omnibox.setMode(OmniboxMode.Command);
Expand Down Expand Up @@ -605,15 +615,34 @@ export class UiMain implements m.ClassComponent {
);
}

oncreate({dom}: m.VnodeDOM) {
this.updateOmniboxInputRef(dom);
// This function is invoked once per trace.
oncreate(vnode: m.VnodeDOM) {
this.updateOmniboxInputRef(vnode.dom);
this.maybeFocusOmnibar();

// Register each command with the command manager
this.cmds.forEach((cmd) => {
const dispose = globals.commandManager.registerCommand(cmd);
this.trash.use(dispose);
this.trash.use(globals.commandManager.registerCommand(cmd));
});

// Register the aggregation tabs.
this.trash.use(new AggregationsTabs());

// Register the notes manager+editor.
this.trash.use(
globals.tabManager.registerDetailsPanel(new NotesEditorTab()),
);

this.trash.use(
globals.tabManager.registerTab({
uri: 'notes.manager',
isEphemeral: false,
content: {
getTitle: () => 'Notes & markers',
render: () => m(NotesListEditor),
},
}),
);
}

onupdate({dom}: m.VnodeDOM) {
Expand All @@ -622,12 +651,24 @@ export class UiMain implements m.ClassComponent {
}

onremove(_: m.VnodeDOM) {
this.trash.dispose();
this.omniboxInputEl = undefined;

// NOTE: if this becomes ever an asyncDispose(), then the promise needs to
// be returned to onbeforeremove, so mithril delays the removal until
// the promise is resolved, but then also the UiMain wrapper needs to be
// more complex to linearize the destruction of the old instane with the
// creation of the new one, without overlaps.
// However, we should not add disposables that issue cleanup queries on the
// Engine. Doing so is: (1) useless: we throw away the whole wasm instance
// on each trace load, so what's the point of deleting tables from a TP
// instance that is going to be destroyed?; (2) harmful: we don't have
// precise linearization with the wasm teardown, so we might end up awaiting
// forever for the asyncDispose() because the query will never run.
this.trash.dispose();
}

private updateOmniboxInputRef(dom: Element): void {
const el = findRef(dom, UiMain.OMNIBOX_INPUT_REF);
const el = findRef(dom, OMNIBOX_INPUT_REF);
if (el && el instanceof HTMLInputElement) {
this.omniboxInputEl = el;
}
Expand Down
6 changes: 6 additions & 0 deletions ui/src/trace_processor/engine.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ export interface TraceProcessorConfig {
}

export interface Engine {
readonly engineId: string;

/**
* Execute a query against the database, returning a promise that resolves
* when the query has completed but rejected when the query fails for whatever
Expand Down Expand Up @@ -502,6 +504,10 @@ export abstract class EngineBase implements Engine {
this.rpcSendRequestBytes(buf);
}

get engineId(): string {
return this.id;
}

getProxy(tag: string): EngineProxy {
return new EngineProxy(this, tag);
}
Expand Down

0 comments on commit 2fadc23

Please sign in to comment.