From 797137a5e84cfb5682ae39b3b7048dbc79d1fe09 Mon Sep 17 00:00:00 2001 From: Steve Golton Date: Mon, 28 Oct 2024 14:38:37 +0000 Subject: [PATCH] ui: Make plugin activation require restart & refactor PluginManager - Activating plugins at runtime is probably flaky, as onTraceLoad happens after other plugins' onTraceReady hooks, so tracks are likely to end up in the wrong order/grouping. - This CL makes it so that enabling a plugin requires a restart of the UI. - This change makes the implementation of PluginManager a bit simpler too so this has been refactored to remove this complexity and generally tidy things up a bit. - Removed plugin manager unit tests as they were not really testing anything. In general it's hard to test the plugin manager, as it relies on global state (flags). Change-Id: I05d88a5596878f8eb248875c2825ab2c7cc7b823 --- .../sched-aggr-process.png.sha256 | 2 +- .../sched-aggr-thread.png.sha256 | 2 +- ui/src/core/plugin_manager.ts | 297 +++++++----------- ui/src/core/plugin_manager_unittest.ts | 68 ---- ui/src/frontend/index.ts | 8 +- ui/src/frontend/plugins_page.ts | 109 ++++--- 6 files changed, 178 insertions(+), 308 deletions(-) delete mode 100644 ui/src/core/plugin_manager_unittest.ts diff --git a/test/data/ui-screenshots/wattson.test.ts/sched-aggregations/sched-aggr-process.png.sha256 b/test/data/ui-screenshots/wattson.test.ts/sched-aggregations/sched-aggr-process.png.sha256 index e82a474520..86f06ee15a 100644 --- a/test/data/ui-screenshots/wattson.test.ts/sched-aggregations/sched-aggr-process.png.sha256 +++ b/test/data/ui-screenshots/wattson.test.ts/sched-aggregations/sched-aggr-process.png.sha256 @@ -1 +1 @@ -7e509757ba16aad63be247ce20bb6fe78f71b36a8be08c8dabca4e1a71bbda1c \ No newline at end of file +aef36afce71de9eaa3fb92d36c2dc37dbfdfc82d9b25717f0b80c11db35d7caa \ No newline at end of file diff --git a/test/data/ui-screenshots/wattson.test.ts/sched-aggregations/sched-aggr-thread.png.sha256 b/test/data/ui-screenshots/wattson.test.ts/sched-aggregations/sched-aggr-thread.png.sha256 index 4ddfae68b3..d51ed7f773 100644 --- a/test/data/ui-screenshots/wattson.test.ts/sched-aggregations/sched-aggr-thread.png.sha256 +++ b/test/data/ui-screenshots/wattson.test.ts/sched-aggregations/sched-aggr-thread.png.sha256 @@ -1 +1 @@ -35b91f67285fd783a9b4f65eb128f6033292cee75751233696f15da9fe1b0ef2 \ No newline at end of file +e0ec62b98f6c2a4d1781ab4b0913f9d11c24b1c5d44b03010fcd178aeda06c3d \ No newline at end of file diff --git a/ui/src/core/plugin_manager.ts b/ui/src/core/plugin_manager.ts index 249cfecc26..209b27da37 100644 --- a/ui/src/core/plugin_manager.ts +++ b/ui/src/core/plugin_manager.ts @@ -13,33 +13,20 @@ // limitations under the License. import {Registry} from '../base/registry'; -import {Trace} from '../public/trace'; import {App} from '../public/app'; -import {MetricVisualisation} from '../public/plugin'; -import {PerfettoPlugin, PluginDescriptor} from '../public/plugin'; -import {Flag, featureFlags} from './feature_flags'; -import {assertExists} from '../base/logging'; -import {raf} from './raf_scheduler'; +import { + MetricVisualisation, + PerfettoPlugin, + PluginDescriptor, +} from '../public/plugin'; +import {Trace} from '../public/trace'; import {defaultPlugins} from './default_plugins'; +import {featureFlags, Flag} from './feature_flags'; import {TraceImpl} from './trace_impl'; // The pseudo plugin id used for the core instance of AppImpl. export const CORE_PLUGIN_ID = '__core__'; -// 'Static' registry of all known plugins. -export class PluginRegistry extends Registry { - constructor() { - super((info) => info.pluginId); - } -} - -export interface PluginDetails { - plugin: PerfettoPlugin; - app: App; - trace?: Trace; - previousOnTraceLoadTimeMillis?: number; -} - function makePlugin(info: PluginDescriptor): PerfettoPlugin { const {plugin} = info; @@ -59,141 +46,88 @@ export interface PluginAppInterface { get trace(): TraceImpl | undefined; } -export class PluginManager { - private registry = new PluginRegistry(); - private _plugins = new Map(); - private flags = new Map(); - private _needsRestart = false; - - constructor(private app: PluginAppInterface) {} +// Contains all the information about a plugin. +export interface PluginWrapper { + // A reference to the plugin descriptor + readonly desc: PluginDescriptor; - get plugins(): Map { - return this._plugins; - } - - // Must only be called once on startup - async initialize(): Promise { - // Check if any plugins in defaultPlugins are not registered - const badDefaults = defaultPlugins.filter((id) => !this.registry.has(id)); - if (badDefaults.length > 0) { - throw new Error(`Missing defaults: ${badDefaults}`); - } + // The feature flag used to allow users to change whether this plugin should + // be enabled or not. + readonly enableFlag: Flag; - for (const {pluginId} of this.registry.values()) { - const flagId = `plugin_${pluginId}`; - const name = `Plugin: ${pluginId}`; - const flag = featureFlags.register({ - id: flagId, - name, - description: `Overrides '${pluginId}' plugin.`, - defaultValue: defaultPlugins.includes(pluginId), - }); - this.flags.set(pluginId, flag); - if (flag.get()) { - await this.activatePlugin(pluginId); - } - } - } - - registerPlugin(descriptor: PluginDescriptor) { - this.registry.register(descriptor); - } - - /** - * Enable plugin flag - i.e. configure a plugin to start on boot. - * @param id The ID of the plugin. - */ - async enablePlugin(id: string): Promise { - const flag = this.flags.get(id); - if (flag) { - flag.set(true); - } - await this.activatePlugin(id); - } - - /** - * Disable plugin flag - i.e. configure a plugin not to start on boot. - * @param id The ID of the plugin. - */ - async disablePlugin(id: string): Promise { - const flag = this.flags.get(id); - if (flag) { - flag.set(false); - } - this._needsRestart = true; - } - - /** - * Start a plugin just for this session. This setting is not persisted. - * @param id The ID of the plugin to start. - */ - async activatePlugin(id: string): Promise { - if (this.isActive(id)) { - return; - } + // If a plugin has been activated, the relevant context is stored here. + activatedContext?: ActivePluginContext; +} - const pluginInfo = this.registry.get(id); - const plugin = makePlugin(pluginInfo); +// Contains an active plugin's contextual information, only created at plugin +// activation time. +interface ActivePluginContext { + // The plugin instance, which is only created at plugin activation time. + readonly pluginInstance: PerfettoPlugin; - const app = this.app.forkForPlugin(id); + // The app interface for this plugin. + readonly app: App; - plugin.onActivate?.(app); + // If a plugin has had its trace loaded, the relevant context is stored here. + traceContext?: TracePluginContext; +} - const pluginDetails: PluginDetails = {plugin, app}; +// Contains the contextual information required by a plugin which has had a +// trace loaded. +interface TracePluginContext { + // The trace interface for this plugin. + readonly trace: Trace; - // If a trace is already loaded when plugin is activated, make sure to - // call onTraceLoad(). - const maybeTrace = this.app.trace; - if (maybeTrace !== undefined) { - await doPluginTraceLoad(pluginDetails, maybeTrace); - await doPluginTraceReady(pluginDetails); - } - - this._plugins.set(id, pluginDetails); + // The time taken in milliseconds to execute this onTraceLoad() function. + readonly loadTimeMs: number; +} - raf.scheduleFullRedraw(); +export class PluginManager { + private readonly registry = new Registry( + (x) => x.desc.pluginId, + ); + + constructor(private readonly app: PluginAppInterface) {} + + registerPlugin(desc: PluginDescriptor) { + const flagId = `plugin_${desc.pluginId}`; + const name = `Plugin: ${desc.pluginId}`; + const flag = featureFlags.register({ + id: flagId, + name, + description: `Overrides '${desc.pluginId}' plugin.`, + defaultValue: defaultPlugins.includes(desc.pluginId), + }); + this.registry.register({ + desc, + enableFlag: flag, + }); } /** - * Restore all plugins enable/disabled flags to their default values. - * Also activates new plugins to match flag settings. + * Activates all registered plugins that have not already been registered. + * + * @param enableOverrides - The list of plugins that are enabled regardless of + * the current flag setting. */ - async restoreDefaults(): Promise { - for (const plugin of this.registry.values()) { - const pluginId = plugin.pluginId; - const flag = assertExists(this.flags.get(pluginId)); - flag.reset(); - if (flag.get()) { - await this.activatePlugin(plugin.pluginId); - } else { - this._needsRestart = true; - } - } - } - - getRegisteredPlugins(): ReadonlyArray { - return this.registry.valuesAsArray(); - } - - hasPlugin(pluginId: string): boolean { - return this.registry.has(pluginId); - } - - isActive(pluginId: string): boolean { - return this.getPluginContext(pluginId) !== undefined; - } - - isEnabled(pluginId: string): boolean { - return Boolean(this.flags.get(pluginId)?.get()); - } - - getPluginContext(pluginId: string): PluginDetails | undefined { - return this._plugins.get(pluginId); + activatePlugins(enableOverrides: ReadonlyArray = []) { + this.registry + .valuesAsArray() + .filter( + (p) => p.enableFlag.get() || enableOverrides.includes(p.desc.pluginId), + ) + .forEach((p) => { + if (p.activatedContext) return; + const pluginInstance = makePlugin(p.desc); + const app = this.app.forkForPlugin(p.desc.pluginId); + pluginInstance.onActivate?.(app); + p.activatedContext = { + pluginInstance, + app, + }; + }); } - // NOTE: here we take as argument the TraceImpl for the core. This is because - // we pass it to doPluginTraceLoad() which uses to call forkForPlugin(id) and - // derive a per-plugin instance. async onTraceLoad( traceCore: TraceImpl, beforeEach?: (id: string) => void, @@ -203,62 +137,61 @@ export class PluginManager { // Running in parallel will have very little performance benefit assuming // most plugins use the same engine, which can only process one query at a // time. - for (const [id, plugin] of this._plugins.entries()) { - beforeEach?.(id); - await doPluginTraceLoad(plugin, traceCore); + for (const p of this.registry.values()) { + const activePlugin = p.activatedContext; + if (activePlugin) { + beforeEach?.(p.desc.pluginId); + const trace = traceCore.forkForPlugin(p.desc.pluginId); + const before = performance.now(); + await activePlugin.pluginInstance.onTraceLoad?.(trace); + const loadTimeMs = performance.now() - before; + activePlugin.traceContext = { + trace, + loadTimeMs: loadTimeMs, + }; + traceCore.trash.defer(() => { + activePlugin.traceContext = undefined; + }); + } } } async onTraceReady(): Promise { - const pluginsShuffled = Array.from(this._plugins.values()) - .map((plugin) => ({plugin, sort: Math.random()})) - .sort((a, b) => a.sort - b.sort); - - for (const {plugin} of pluginsShuffled) { - await doPluginTraceReady(plugin); + for (const plugin of this.registry.values()) { + const activePlugin = plugin.activatedContext; + if (activePlugin) { + const traceContext = activePlugin.traceContext; + if (traceContext) { + await activePlugin.pluginInstance?.onTraceReady?.(traceContext.trace); + } + } } } metricVisualisations(): MetricVisualisation[] { - return Array.from(this._plugins.values()).flatMap((ctx) => { - const tracePlugin = ctx.plugin; - if (tracePlugin.metricVisualisations) { - return tracePlugin.metricVisualisations(ctx.app); + return this.registry.valuesAsArray().flatMap((plugin) => { + const activePlugin = plugin.activatedContext; + if (activePlugin) { + return ( + activePlugin.pluginInstance.metricVisualisations?.( + activePlugin.app, + ) ?? [] + ); } else { return []; } }); } - get needsRestart() { - return this._needsRestart; + getAllPlugins() { + return this.registry.valuesAsArray(); } -} - -async function doPluginTraceReady(pluginDetails: PluginDetails): Promise { - const {plugin, trace: traceContext} = pluginDetails; - await Promise.resolve(plugin.onTraceReady?.(assertExists(traceContext))); - raf.scheduleFullRedraw(); -} - -async function doPluginTraceLoad( - pluginDetails: PluginDetails, - traceCore: TraceImpl, -): Promise { - const {plugin} = pluginDetails; - const trace = traceCore.forkForPlugin(pluginDetails.app.pluginId); - pluginDetails.trace = trace; - - const startTime = performance.now(); - await Promise.resolve(plugin.onTraceLoad?.(trace)); - const loadTime = performance.now() - startTime; - pluginDetails.previousOnTraceLoadTimeMillis = loadTime; - - traceCore.trash.defer(() => { - pluginDetails.trace = undefined; - pluginDetails.previousOnTraceLoadTimeMillis = undefined; - }); + getPluginContainer(id: string): PluginWrapper | undefined { + return this.registry.tryGet(id); + } - raf.scheduleFullRedraw(); + getPlugin(id: string): PerfettoPlugin | undefined { + return this.registry.tryGet(id)?.activatedContext?.pluginInstance; + } } diff --git a/ui/src/core/plugin_manager_unittest.ts b/ui/src/core/plugin_manager_unittest.ts deleted file mode 100644 index c56ba14fdb..0000000000 --- a/ui/src/core/plugin_manager_unittest.ts +++ /dev/null @@ -1,68 +0,0 @@ -// Copyright (C) 2023 The Android Open Source Project -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -import {PerfettoPlugin} from '../public/plugin'; -import {AppImpl} from './app_impl'; -import { - createFakeTraceImpl, - initializeAppImplForTesting, -} from './fake_trace_impl'; -import {PluginManager} from './plugin_manager'; - -function makeMockPlugin(): PerfettoPlugin { - return { - onActivate: jest.fn(), - onTraceLoad: jest.fn(), - }; -} - -let mockPlugin: PerfettoPlugin; -let manager: PluginManager; - -describe('PluginManger', () => { - beforeEach(() => { - mockPlugin = makeMockPlugin(); - initializeAppImplForTesting(); - manager = new PluginManager(initializeAppImplForTesting()); - manager.registerPlugin({ - pluginId: 'foo', - plugin: mockPlugin, - }); - }); - - it('can activate plugin', async () => { - await manager.activatePlugin('foo'); - - expect(manager.isActive('foo')).toBe(true); - expect(mockPlugin.onActivate).toHaveBeenCalledTimes(1); - }); - - it('invokes onTraceLoad when trace is loaded', async () => { - const trace = createFakeTraceImpl(); - AppImpl.instance.setActiveTrace(trace); - await manager.onTraceLoad(trace); - await manager.activatePlugin('foo'); - - expect(mockPlugin.onTraceLoad).toHaveBeenCalledTimes(1); - }); - - it('invokes onTraceLoad when plugin activated while trace loaded', async () => { - const trace = createFakeTraceImpl(); - AppImpl.instance.setActiveTrace(trace); - await manager.onTraceLoad(trace); - await manager.activatePlugin('foo'); - - expect(mockPlugin.onTraceLoad).toHaveBeenCalledTimes(1); - }); -}); diff --git a/ui/src/frontend/index.ts b/ui/src/frontend/index.ts index 93f0c71974..88631558e1 100644 --- a/ui/src/frontend/index.ts +++ b/ui/src/frontend/index.ts @@ -381,13 +381,9 @@ function onCssLoaded() { const pluginManager = AppImpl.instance.plugins; CORE_PLUGINS.forEach((p) => pluginManager.registerPlugin(p)); NON_CORE_PLUGINS.forEach((p) => pluginManager.registerPlugin(p)); - pluginManager.initialize(); const route = Router.parseUrl(window.location.href); - for (const pluginId of (route.args.enablePlugins ?? '').split(',')) { - if (pluginManager.hasPlugin(pluginId)) { - pluginManager.activatePlugin(pluginId); - } - } + const overrides = (route.args.enablePlugins ?? '').split(','); + pluginManager.activatePlugins(overrides); } // If the URL is /#!?rpc_port=1234, change the default RPC port. diff --git a/ui/src/frontend/plugins_page.ts b/ui/src/frontend/plugins_page.ts index 30f70302e7..732a8d06ab 100644 --- a/ui/src/frontend/plugins_page.ts +++ b/ui/src/frontend/plugins_page.ts @@ -13,23 +13,28 @@ // limitations under the License. import m from 'mithril'; -import {raf} from '../core/raf_scheduler'; import {Button} from '../widgets/button'; import {exists} from '../base/utils'; -import {PluginDescriptor} from '../public/plugin'; import {defaultPlugins} from '../core/default_plugins'; import {Intent} from '../widgets/common'; import {PageAttrs} from '../core/router'; import {AppImpl} from '../core/app_impl'; +import {PluginWrapper} from '../core/plugin_manager'; +import {raf} from '../core/raf_scheduler'; + +// This flag indicated whether we need to restart the UI to apply plugin +// changes. It is purposely a global as we want it to outlive the Mithril +// component, and it'll be reset we restart anyway. +let needsRestart = false; export class PluginsPage implements m.ClassComponent { view() { const pluginManager = AppImpl.instance.plugins; - const registeredPlugins = pluginManager.getRegisteredPlugins(); + const registeredPlugins = pluginManager.getAllPlugins(); return m( '.pf-plugins-page', m('h1', 'Plugins'), - pluginManager.needsRestart && + needsRestart && m( 'h3.restart_needed', 'Some plugins have been disabled. ' + @@ -42,9 +47,10 @@ export class PluginsPage implements m.ClassComponent { label: 'Disable All', onclick: async () => { for (const plugin of registeredPlugins) { - await pluginManager.disablePlugin(plugin.pluginId); - raf.scheduleFullRedraw(); + plugin.enableFlag.set(false); } + needsRestart = true; + raf.scheduleFullRedraw(); }, }), m(Button, { @@ -52,65 +58,68 @@ export class PluginsPage implements m.ClassComponent { label: 'Enable All', onclick: async () => { for (const plugin of registeredPlugins) { - await pluginManager.enablePlugin(plugin.pluginId); - raf.scheduleFullRedraw(); + plugin.enableFlag.set(true); } + needsRestart = true; + raf.scheduleFullRedraw(); }, }), m(Button, { intent: Intent.Primary, label: 'Restore Defaults', onclick: async () => { - await pluginManager.restoreDefaults(); + for (const plugin of registeredPlugins) { + plugin.enableFlag.reset(); + } + needsRestart = true; raf.scheduleFullRedraw(); }, }), ), m( '.pf-plugins-grid', - [ - m('span', 'Plugin'), - m('span', 'Default?'), - m('span', 'Enabled?'), - m('span', 'Active?'), - m('span', 'Control'), - m('span', 'Load Time'), - ], - registeredPlugins.map((plugin) => renderPluginRow(plugin)), + m('span', 'Plugin'), + m('span', 'Default?'), + m('span', 'Enabled?'), + m('span', 'Active?'), + m('span', 'Control'), + m('span', 'Load Time'), + registeredPlugins.map((plugin) => this.renderPluginRow(plugin)), ), ); } -} -function renderPluginRow(plugin: PluginDescriptor): m.Children { - const pluginManager = AppImpl.instance.plugins; - const pluginId = plugin.pluginId; - const isDefault = defaultPlugins.includes(pluginId); - const pluginDetails = pluginManager.plugins.get(pluginId); - const isActive = pluginManager.isActive(pluginId); - const isEnabled = pluginManager.isEnabled(pluginId); - const loadTime = pluginDetails?.previousOnTraceLoadTimeMillis; - return [ - m('span', pluginId), - m('span', isDefault ? 'Yes' : 'No'), - isEnabled - ? m('.pf-tag.pf-active', 'Enabled') - : m('.pf-tag.pf-inactive', 'Disabled'), - isActive - ? m('.pf-tag.pf-active', 'Active') - : m('.pf-tag.pf-inactive', 'Inactive'), - m(Button, { - label: isEnabled ? 'Disable' : 'Enable', - intent: Intent.Primary, - onclick: async () => { - if (isEnabled) { - await pluginManager.disablePlugin(pluginId); - } else { - await pluginManager.enablePlugin(pluginId); - } - raf.scheduleFullRedraw(); - }, - }), - exists(loadTime) ? m('span', `${loadTime.toFixed(1)} ms`) : m('span', `-`), - ]; + private renderPluginRow(plugin: PluginWrapper): m.Children { + const pluginId = plugin.desc.pluginId; + const isDefault = defaultPlugins.includes(pluginId); + const isActive = plugin.activatedContext; + const isEnabled = plugin.enableFlag.get(); + const loadTime = plugin.activatedContext?.traceContext?.loadTimeMs; + return [ + m('span', pluginId), + m('span', isDefault ? 'Yes' : 'No'), + isEnabled + ? m('.pf-tag.pf-active', 'Enabled') + : m('.pf-tag.pf-inactive', 'Disabled'), + isActive + ? m('.pf-tag.pf-active', 'Active') + : m('.pf-tag.pf-inactive', 'Inactive'), + m(Button, { + label: isEnabled ? 'Disable' : 'Enable', + intent: Intent.Primary, + onclick: () => { + if (isEnabled) { + plugin.enableFlag.set(false); + } else { + plugin.enableFlag.set(true); + } + needsRestart = true; + raf.scheduleFullRedraw(); + }, + }), + exists(loadTime) + ? m('span', `${loadTime.toFixed(1)} ms`) + : m('span', `-`), + ]; + } }