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

@@ -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<string> = [];
+  sdkVersion: string | undefined = undefined;
+  pidAppMapping: {[key: number]: string} = {};
+  private recordingProcess?: Promise<string>;
+  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;
+  stopLogging() {
-  abiList: Array<string> = [];
+    this.reader?.end();
-  sdkVersion: string | undefined = undefined;
+  }
-  pidAppMapping: {[key: number]: string} = {};
-  private recordingProcess?: Promise<string>;
 
   reverse(ports: [number, number]): Promise<void> {
     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) {

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);
   }
 

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);
+});

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,

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);

desktop/static/icons.json

@@ -582,5 +582,8 @@
   ],
   "app-microsoft-windows": [
     24
+  ],
+  "box-outline": [
+    24
   ]
 }