From d293b2b0e588dee6ec1aff45042442975ed865a0 Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Tue, 16 Mar 2021 14:54:53 -0700 Subject: [PATCH] Dispose adb logcat connection if there are no listeners Summary: Flipper used to always subscribe to the adb logs. This change makes the log subscription lazy, which means if there are no plugins listening to the logs, we don't subscribe to the adb logs at all. As you can see in the recording (prod build) this lowers CPU usage significantly: without logs plugin disabled it is now ~5-8 %, with the plugin enabled ~12-18%. Before this change we would never stop listening to the log output, even when the plugins were disabled, causing a constant background noise which people complain regularly about. The pause / resume button in the new log plugin will now cleanup the connection as well, so that a simple 'pause' click will already boost performance, without needing to disable the logs plugin (if crash reporter is not enabled) In this diff we also will clear the logs when reconnected. Previously we didn't reset the logs after connecting, we means Flipper would try to gets up with all past logs first, which could be a few 100 K entries. Further future optimizations could involve using logcat smarter; by actually passing filters on app or loglevel to the command (not sure if that is actually faster, or just a convenience api that does the same filtering as we do) Changelog: Flipper will now use less CPU if logs & crash reporter plugins are disabled by no longer tailing adb logcat. Reviewed By: nikoant Differential Revision: D27047041 fbshipit-source-id: 251a04dcc6f488f220cb56fe50a26788d795e38c --- desktop/app/src/devices/AndroidDevice.tsx | 30 +++- desktop/app/src/devices/BaseDevice.tsx | 15 ++ .../src/devices/__tests__/BaseDevice.node.tsx | 134 ++++++++++++++++++ desktop/app/src/test-utils/MockFlipper.tsx | 5 + .../createMockFlipperWithPlugin.tsx | 8 +- desktop/static/icons.json | 3 + 6 files changed, 186 insertions(+), 9 deletions(-) diff --git a/desktop/app/src/devices/AndroidDevice.tsx b/desktop/app/src/devices/AndroidDevice.tsx index 9970b51a7..59557eb2e 100644 --- a/desktop/app/src/devices/AndroidDevice.tsx +++ b/desktop/app/src/devices/AndroidDevice.tsx @@ -9,7 +9,7 @@ import BaseDevice from './BaseDevice'; import adb, {Client as ADBClient} from 'adbkit'; -import {Priority} from 'adbkit-logcat'; +import {Priority, Reader} from 'adbkit-logcat'; import {createWriteStream} from 'fs'; import type {LogLevel, DeviceType} from 'flipper-plugin'; import which from 'which'; @@ -20,6 +20,13 @@ import {DeviceSpec} from 'flipper-plugin-lib'; const DEVICE_RECORDING_DIR = '/sdcard/flipper_recorder'; export default class AndroidDevice extends BaseDevice { + adb: ADBClient; + abiList: Array = []; + sdkVersion: string | undefined = undefined; + pidAppMapping: {[key: number]: string} = {}; + private recordingProcess?: Promise; + reader?: Reader; + constructor( serial: string, deviceType: DeviceType, @@ -34,7 +41,11 @@ export default class AndroidDevice extends BaseDevice { this.icon = 'mobile'; this.abiList = abiList; this.sdkVersion = sdkVersion; - this.adb.openLogcat(this.serial).then((reader) => { + } + + startLogging() { + this.adb.openLogcat(this.serial, {clear: true}).then((reader) => { + this.reader = reader; reader.on('entry', (entry) => { let type: LogLevel = 'unknown'; if (entry.priority === Priority.VERBOSE) { @@ -68,11 +79,9 @@ export default class AndroidDevice extends BaseDevice { }); } - adb: ADBClient; - abiList: Array = []; - sdkVersion: string | undefined = undefined; - pidAppMapping: {[key: number]: string} = {}; - private recordingProcess?: Promise; + stopLogging() { + this.reader?.end(); + } reverse(ports: [number, number]): Promise { return Promise.all( @@ -217,6 +226,13 @@ export default class AndroidDevice extends BaseDevice { this.recordingProcess = undefined; return destination; } + + disconnect() { + if (this.recordingProcess) { + this.stopScreenCapture(); + } + super.disconnect(); + } } export async function launchEmulator(name: string, coldBoot: boolean = false) { diff --git a/desktop/app/src/devices/BaseDevice.tsx b/desktop/app/src/devices/BaseDevice.tsx index c601d8296..878e83e28 100644 --- a/desktop/app/src/devices/BaseDevice.tsx +++ b/desktop/app/src/devices/BaseDevice.tsx @@ -131,7 +131,18 @@ export default class BaseDevice { }; } + startLogging() { + // to be subclassed + } + + stopLogging() { + // to be subclassed + } + addLogListener(callback: DeviceLogListener): Symbol { + if (this.logListeners.size === 0) { + this.startLogging(); + } const id = Symbol(); this.logListeners.set(id, callback); return id; @@ -156,6 +167,9 @@ export default class BaseDevice { removeLogListener(id: Symbol) { this.logListeners.delete(id); + if (this.logListeners.size === 0) { + this.stopLogging(); + } } navigateToLocation(_location: string) { @@ -268,6 +282,7 @@ export default class BaseDevice { disconnect() { this.logListeners.clear(); + this.stopLogging(); this.connected.set(false); } diff --git a/desktop/app/src/devices/__tests__/BaseDevice.node.tsx b/desktop/app/src/devices/__tests__/BaseDevice.node.tsx index fe3d95cf6..d2fd866b4 100644 --- a/desktop/app/src/devices/__tests__/BaseDevice.node.tsx +++ b/desktop/app/src/devices/__tests__/BaseDevice.node.tsx @@ -12,6 +12,7 @@ import * as DeviceTestPluginModule from '../../test-utils/DeviceTestPlugin'; import {TestUtils, _SandyPluginDefinition} from 'flipper-plugin'; import ArchivedDevice from '../ArchivedDevice'; import DummyDevice from '../DummyDevice'; +import {createMockFlipperWithPlugin} from '../../test-utils/createMockFlipperWithPlugin'; const physicalDevicePluginDetails = TestUtils.createMockPluginDetails({ id: 'physicalDevicePlugin', @@ -178,3 +179,136 @@ test('android dummy device compatibility', () => { expect(device.supportsPlugin(androidEmulatorDevicePlugin)).toBeFalsy(); expect(device.supportsPlugin(androidOnlyDevicePlugin)).toBeTruthy(); }); + +test('log listeners are resumed and suspended automatically - 1', async () => { + const message = { + date: new Date(), + message: 'test', + pid: 0, + tid: 1, + type: 'info', + tag: 'tag', + } as const; + const device = new BaseDevice('serial', 'physical', 'test device', 'Android'); + device.startLogging = jest.fn(); + device.stopLogging = jest.fn(); + + const DevicePlugin = TestUtils.createTestDevicePlugin({ + devicePlugin(client) { + const entries: any[] = []; + let disposer: any; + + function start() { + disposer = client.device.onLogEntry((entry) => { + entries.push(entry); + }); + } + function stop() { + disposer?.(); + } + + start(); + + return {start, stop, entries}; + }, + }); + + await createMockFlipperWithPlugin(DevicePlugin, { + device, + }); + const instance = device.sandyPluginStates.get(DevicePlugin.id); + expect(instance).toBeDefined(); + const entries = instance?.instanceApi.entries as any[]; + + // logging set up, messages arrive + expect(device.startLogging).toBeCalledTimes(1); + device.addLogEntry(message); + expect(entries.length).toBe(1); + + // stop, messages don't arrive + instance?.instanceApi.stop(); + expect(device.stopLogging).toBeCalledTimes(1); + device.addLogEntry(message); + expect(entries.length).toBe(1); + + // resume, messsages arrive again + instance?.instanceApi.start(); + expect(device.startLogging).toBeCalledTimes(2); + expect(device.stopLogging).toBeCalledTimes(1); + device.addLogEntry(message); + expect(entries.length).toBe(2); + + // device disconnects, loggers are disposed + device.disconnect(); + expect(device.stopLogging).toBeCalledTimes(2); +}); + +test('log listeners are resumed and suspended automatically - 2', async () => { + const message = { + date: new Date(), + message: 'test', + pid: 0, + tid: 1, + type: 'info', + tag: 'tag', + } as const; + const device = new BaseDevice('serial', 'physical', 'test device', 'Android'); + device.startLogging = jest.fn(); + device.stopLogging = jest.fn(); + + const entries: any[] = []; + + const DevicePlugin = TestUtils.createTestDevicePlugin({ + devicePlugin(client) { + client.device.onLogEntry((entry) => { + entries.push(entry); + }); + return {}; + }, + }); + + const Plugin = TestUtils.createTestPlugin( + { + plugin(client) { + client.device.onLogEntry((entry) => { + entries.push(entry); + }); + return {}; + }, + }, + { + id: 'AnotherPlugin', + }, + ); + + const flipper = await createMockFlipperWithPlugin(DevicePlugin, { + device, + additionalPlugins: [Plugin], + }); + const instance = device.sandyPluginStates.get(DevicePlugin.id); + expect(instance).toBeDefined(); + + // logging set up, messages arrives in both + expect(device.startLogging).toBeCalledTimes(1); + device.addLogEntry(message); + expect(entries.length).toBe(2); + + // disable one plugin + flipper.togglePlugin(Plugin.id); + expect(device.stopLogging).toBeCalledTimes(0); + device.addLogEntry(message); + expect(entries.length).toBe(3); + + // disable the other plugin + flipper.togglePlugin(DevicePlugin.id); + + expect(device.stopLogging).toBeCalledTimes(1); + device.addLogEntry(message); + expect(entries.length).toBe(3); + + // re-enable plugn + flipper.togglePlugin(Plugin.id); + expect(device.startLogging).toBeCalledTimes(2); + device.addLogEntry(message); + expect(entries.length).toBe(4); +}); diff --git a/desktop/app/src/test-utils/MockFlipper.tsx b/desktop/app/src/test-utils/MockFlipper.tsx index f0ac7ccc8..baa30ba3e 100644 --- a/desktop/app/src/test-utils/MockFlipper.tsx +++ b/desktop/app/src/test-utils/MockFlipper.tsx @@ -120,6 +120,11 @@ export default class MockFlipper { device.supportsPlugin = !isSupportedByPlugin ? () => true : isSupportedByPlugin; + this.loadDevice(device); + return device; + } + + public loadDevice(device: BaseDevice) { this._store.dispatch({ type: 'REGISTER_DEVICE', payload: device, diff --git a/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx b/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx index 8826a2188..1758cc214 100644 --- a/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx +++ b/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx @@ -61,6 +61,7 @@ type MockOptions = Partial<{ dontEnableAdditionalPlugins?: true; asBackgroundPlugin?: true; supportedPlugins?: string[]; + device?: BaseDevice; }>; function isPluginEnabled( @@ -128,7 +129,9 @@ export async function createMockFlipperWithPlugin( return client; }; - const device = createDevice('serial'); + const device = options?.device + ? mockFlipper.loadDevice(options?.device) + : createDevice('serial'); const client = await createClient(device, 'TestApp'); store.dispatch(selectDevice(device)); @@ -172,7 +175,8 @@ export async function createMockFlipperWithPlugin( pluginKey: getPluginKey(client.id, device, pluginClazz.id), togglePlugin(id?: string) { const plugin = id - ? store.getState().plugins.clientPlugins.get(id) + ? store.getState().plugins.clientPlugins.get(id) ?? + store.getState().plugins.devicePlugins.get(id) : pluginClazz; if (!plugin) { throw new Error('unknown plugin ' + id); diff --git a/desktop/static/icons.json b/desktop/static/icons.json index 070e80c0d..b3b8d1552 100644 --- a/desktop/static/icons.json +++ b/desktop/static/icons.json @@ -582,5 +582,8 @@ ], "app-microsoft-windows": [ 24 + ], + "box-outline": [ + 24 ] } \ No newline at end of file