Store clients as Map rather than array

Summary: Refactor clients storage: array -> map. A lot of logic looks up clients by their id, which is currently done with an array.find operation, which is pretty inefficient. This diff changes it to a map, that is pretty important, as in the next diff the decoupled client message handing will need to find the client again for every message that arrives.

Reviewed By: timur-valiev

Differential Revision: D31303536

fbshipit-source-id: ca3f540a3de7665930d2354436d37cb0fbfd5546

desktop/app/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap

@@ -2,8 +2,8 @@
 
 exports[`can create a Fake flipper with legacy wrapper 1`] = `
 Object {
-  "clients": Array [
-    Object {
+  "clients": Map {
+    "TestApp#Android#MockAndroidDevice#serial" => Object {
       "id": "TestApp#Android#MockAndroidDevice#serial",
       "query": Object {
         "app": "TestApp",
@@ -13,7 +13,7 @@ Object {
         "sdk_version": 4,
       },
     },
-  ],
+  },
   "deepLinkPayload": null,
   "devices": Array [
     Object {

desktop/app/src/__tests__/createMockFlipperWithPlugin.node.tsx

@@ -10,6 +10,7 @@
 import {createMockFlipperWithPlugin} from '../test-utils/createMockFlipperWithPlugin';
 import {FlipperPlugin} from '../plugin';
 import {TestIdler} from '../utils/Idler';
+import {getAllClients} from '../reducers/connections';
 
 interface PersistedState {
   count: 1;
@@ -60,8 +61,8 @@ test('can create a Fake flipper with legacy wrapper', async () => {
   expect(state.plugins).toMatchSnapshot();
   sendMessage('inc', {});
   expect(
-    await state.connections.clients[0].sandyPluginStates
-      .get(TestPlugin.id)!
+    await getAllClients(state.connections)[0]
+      .sandyPluginStates.get(TestPlugin.id)!
       .exportState(testIdler, testOnStatusMessage),
   ).toMatchInlineSnapshot(`"{\\"count\\":1}"`);
 });

desktop/app/src/chrome/plugin-manager/PluginDebugger.tsx

@@ -41,7 +41,7 @@ type StateFromProps = {
   gatekeepedPlugins: Array<PluginDetails>;
   disabledPlugins: Array<PluginDetails>;
   failedPlugins: Array<[PluginDetails, string]>;
-  clients: Array<Client>;
+  clients: Map<string, Client>;
   selectedDevice: string | null | undefined;
   devicePlugins: PluginDefinition[];
   clientPlugins: PluginDefinition[];
@@ -126,7 +126,7 @@ class PluginDebugger extends Component<Props> {
   }
 
   getSupportedClients(id: string): string {
-    return this.props.clients
+    return Array.from(this.props.clients.values())
       .reduce((acc: Array<string>, cv: Client) => {
         if (cv.plugins.has(id)) {
           acc.push(cv.query.app);

desktop/app/src/deeplink.tsx

@@ -16,7 +16,7 @@ import {Group, SUPPORTED_GROUPS} from './reducers/supportForm';
 import {Logger} from './fb-interfaces/Logger';
 import {Store} from './reducers/index';
 import {importDataToStore} from './utils/exportData';
-import {selectPlugin} from './reducers/connections';
+import {selectPlugin, getAllClients} from './reducers/connections';
 import {Dialog} from 'flipper-plugin';
 import {handleOpenPluginDeeplink} from './dispatcher/handleOpenPluginDeeplink';
 import {message} from 'antd';
@@ -124,13 +124,11 @@ export async function handleDeeplink(
       : undefined;
 
     // if a client is specified, find it, withing the device if applicable
-    const selectedClient = store
-      .getState()
-      .connections.clients.find(
-        (c) =>
-          c.query.app === match[0] &&
-          (selectedDevice == null || c.device === selectedDevice),
-      );
+    const selectedClient = getAllClients(store.getState().connections).find(
+      (c) =>
+        c.query.app === match[0] &&
+        (selectedDevice == null || c.device === selectedDevice),
+    );
 
     store.dispatch(
       selectPlugin({

desktop/app/src/dispatcher/flipperServer.tsx

@@ -144,7 +144,7 @@ export default async (store: Store, logger: Logger) => {
 
 export async function handleClientConnected(store: Store, client: Client) {
   const {connections} = store.getState();
-  const existingClient = connections.clients.find((c) => c.id === client.id);
+  const existingClient = connections.clients.get(client.id);
 
   if (existingClient) {
     existingClient.destroy();

desktop/app/src/dispatcher/handleOpenPluginDeeplink.tsx

@@ -28,6 +28,7 @@ import BaseDevice from '../devices/BaseDevice';
 import Client from '../Client';
 import {RocketOutlined} from '@ant-design/icons';
 import {showEmulatorLauncher} from '../sandy-chrome/appinspect/LaunchEmulator';
+import {getAllClients} from '../reducers/connections';
 
 type OpenPluginParams = {
   pluginId: string;
@@ -432,9 +433,8 @@ async function selectDevicesAndClient(
 
   // wait for valid client
   while (true) {
-    const validClients = store
-      .getState()
-      .connections.clients.filter(
+    const validClients = getAllClients(store.getState().connections)
+      .filter(
         // correct app name, or, if not set, an app that at least supports this plugin
         (c) =>
           params.client

desktop/app/src/dispatcher/pluginDownloads.tsx

@@ -30,6 +30,7 @@ import {reportPlatformFailures, reportUsage} from '../utils/metrics';
 import {loadPlugin} from '../reducers/pluginManager';
 import {showErrorNotification} from '../utils/notifications';
 import {pluginInstalled} from '../reducers/plugins';
+import {getAllClients} from '../reducers/connections';
 
 // Adapter which forces node.js implementation for axios instead of browser implementation
 // used by default in Electron. Node.js implementation is better, because it
@@ -167,7 +168,7 @@ function pluginIsDisabledForAllConnectedClients(
 ) {
   return (
     !state.plugins.clientPlugins.has(plugin.id) ||
-    !state.connections.clients.some((c) =>
+    !getAllClients(state.connections).some((c) =>
       state.connections.enabledPlugins[c.query.app]?.includes(plugin.id),
     )
   );

desktop/app/src/dispatcher/pluginManager.tsx

@@ -40,6 +40,8 @@ import {
   setDevicePluginDisabled,
   setPluginEnabled,
   setPluginDisabled,
+  getClientsByAppName,
+  getAllClients,
 } from '../reducers/connections';
 import {deconstructClientId} from '../utils/clientUtils';
 import {clearMessageQueue} from '../reducers/pluginMessageQueue';
@@ -195,9 +197,7 @@ function switchClientPlugin(
     return;
   }
   const {connections} = store.getState();
-  const clients = connections.clients.filter(
-    (client) => client.query.app === selectedApp,
-  );
+  const clients = getClientsByAppName(connections.clients, selectedApp);
   if (connections.enabledPlugins[selectedApp]?.includes(plugin.id)) {
     clients.forEach((client) => {
       stopPlugin(client, plugin.id);
@@ -240,7 +240,7 @@ function updateClientPlugin(
   plugin: PluginDefinition,
   enable: boolean,
 ) {
-  const clients = store.getState().connections.clients;
+  const clients = getAllClients(store.getState().connections);
   if (enable) {
     const selectedApp = getSelectedAppName(store);
     if (selectedApp) {

desktop/app/src/dispatcher/tracking.tsx

@@ -15,7 +15,6 @@ import {EventEmitter} from 'events';
 
 import {State, Store} from '../reducers/index';
 import {Logger} from '../fb-interfaces/Logger';
-import Client from '../Client';
 import {
   getPluginBackgroundStats,
   resetPluginBackgroundStatsDelta,
@@ -225,7 +224,7 @@ export default (store: Store, logger: Logger) => {
     let sdkVersion: number | null = null;
 
     if (selectedAppId) {
-      const client = clients.find((c: Client) => c.id === selectedAppId);
+      const client = clients.get(selectedAppId);
       if (client) {
         app = client.query.app;
         sdkVersion = client.query.sdk_version || 0;

desktop/app/src/reducers/__tests__/connections.node.tsx

@@ -128,7 +128,7 @@ test('can handle plugins that throw at start', async () => {
   // not initialized
   expect(client.sandyPluginStates.get(TestPlugin.id)).toBe(undefined);
 
-  expect(store.getState().connections.clients.length).toBe(1);
+  expect(store.getState().connections.clients.size).toBe(1);
   expect(client.connected.get()).toBe(true);
 
   expect((console.error as any).mock.calls[0]).toMatchInlineSnapshot(`
@@ -147,7 +147,7 @@ test('can handle plugins that throw at start', async () => {
       [Error: Broken plugin],
     ]
   `);
-  expect(store.getState().connections.clients.length).toBe(2);
+  expect(store.getState().connections.clients.size).toBe(2);
   expect(client2.connected.get()).toBe(true);
   expect(client2.sandyPluginStates.size).toBe(0);
 });

desktop/app/src/reducers/connections.tsx

@@ -70,7 +70,7 @@ type StateV2 = {
   userPreferredApp: null | string; // The name of the preferred app, e.g. Facebook
   enabledPlugins: {[client: string]: string[]};
   enabledDevicePlugins: Set<string>;
-  clients: Array<Client>;
+  clients: Map<string, Client>;
   uninitializedClients: UninitializedClient[];
   deepLinkPayload: unknown;
   staticView: StaticView;
@@ -181,7 +181,7 @@ const INITAL_STATE: State = {
     'Hermesdebuggerrn',
     'React',
   ]),
-  clients: [],
+  clients: new Map(),
   uninitializedClients: [],
   deepLinkPayload: null,
   staticView: WelcomeScreenStaticView,
@@ -253,14 +253,14 @@ export default (state: State = INITAL_STATE, action: Actions): State => {
       if (selectNewDevice) {
         // need to select a different app
         selectedAppId =
-          state.clients.find(
+          getAllClients(state).find(
             (c) =>
               c.device === payload && c.query.app === state.userPreferredApp,
           )?.id ?? null;
         // nothing found, try first app if any
         if (!selectedAppId) {
           selectedAppId =
-            state.clients.find((c) => c.device === payload)?.id ?? null;
+            getAllClients(state).find((c) => c.device === payload)?.id ?? null;
         }
       }
 
@@ -279,7 +279,7 @@ export default (state: State = INITAL_STATE, action: Actions): State => {
         performance.mark(`activePlugin-${selectedPlugin}`);
       }
 
-      const client = state.clients.find((c) => c.id === selectedAppId);
+      const client = state.clients.get(selectedAppId!);
       const device = action.payload.selectedDevice ?? client?.device;
 
       if (!device) {
@@ -298,7 +298,7 @@ export default (state: State = INITAL_STATE, action: Actions): State => {
           : state.userPreferredDevice,
         selectedAppId: selectedAppId ?? null,
         userPreferredApp:
-          state.clients.find((c) => c.id === selectedAppId)?.query.app ??
+          state.clients.get(selectedAppId!)?.query.app ??
           state.userPreferredApp,
         selectedPlugin,
         userPreferredPlugin: selectedPlugin,
@@ -309,42 +309,38 @@ export default (state: State = INITAL_STATE, action: Actions): State => {
     case 'NEW_CLIENT': {
       const {payload} = action;
 
-      const newClients = state.clients.filter((client) => {
-        if (client.id === payload.id) {
+      return produce(state, (draft) => {
+        if (draft.clients.has(payload.id)) {
           console.error(
-            `Received a new connection for client ${client.id}, but the old connection was not cleaned up`,
+            `Received a new connection for client ${payload.id}, but the old connection was not cleaned up`,
           );
-          return false;
         }
-        return true;
+        draft.clients.set(payload.id, payload);
+
+        // select new client if nothing select, this one is preferred, or the old one is offline
+        const selectNewClient =
+          !draft.selectedAppId ||
+          draft.userPreferredApp === payload.query.app ||
+          draft.clients.get(draft.selectedAppId!)?.connected.get() === false;
+
+        if (selectNewClient) {
+          draft.selectedAppId = payload.id;
+          draft.selectedDevice = payload.device;
+        }
+
+        const unitialisedIndex = draft.uninitializedClients.findIndex(
+          (c) =>
+            c.deviceName === payload.query.device ||
+            c.appName === payload.query.app,
+        );
+        if (unitialisedIndex !== -1)
+          draft.uninitializedClients.splice(unitialisedIndex, 1);
       });
-      newClients.push(payload);
-
-      // select new client if nothing select, this one is preferred, or the old one is offline
-      const selectNewClient =
-        !state.selectedAppId ||
-        state.userPreferredApp === payload.query.app ||
-        state.clients
-          .find((c) => c.id === state.selectedAppId)
-          ?.connected.get() === false;
-
-      return {
-        ...state,
-        selectedAppId: selectNewClient ? payload.id : state.selectedAppId,
-        selectedDevice: selectNewClient ? payload.device : state.selectedDevice,
-        clients: newClients,
-        uninitializedClients: state.uninitializedClients.filter((c) => {
-          return (
-            c.deviceName !== payload.query.device ||
-            c.appName !== payload.query.app
-          );
-        }),
-      };
     }
 
     case 'SELECT_CLIENT': {
       const {payload} = action;
-      const client = state.clients.find((c) => c.id === payload);
+      const client = state.clients.get(payload);
 
       if (!client) {
         return state;
@@ -369,15 +365,12 @@ export default (state: State = INITAL_STATE, action: Actions): State => {
     case 'CLIENT_REMOVED': {
       const {payload} = action;
 
-      const newClients = state.clients.filter(
-        (client) => client.id !== payload,
-      );
-      return {
-        ...state,
-        selectedAppId:
-          state.selectedAppId === payload ? null : state.selectedAppId,
-        clients: newClients,
-      };
+      return produce(state, (draft) => {
+        draft.clients.delete(payload);
+        if (draft.selectedAppId === payload) {
+          draft.selectedAppId = null;
+        }
+      });
     }
 
     case 'START_CLIENT_SETUP': {
@@ -522,31 +515,25 @@ export const appPluginListChanged = (): Action => ({
   type: 'APP_PLUGIN_LIST_CHANGED',
 });
 
-export function getAvailableClients(
+export function getClientsByDevice(
   device: null | undefined | BaseDevice,
-  clients: Client[],
+  clients: Map<string, Client>,
 ): Client[] {
   if (!device) {
     return [];
   }
-  return clients
-    .filter(
-      (client: Client) =>
-        (device &&
-          device.supportsOS(client.query.os) &&
-          client.query.device_id === device.serial) ||
-        // Old android sdk versions don't know their device_id
-        // Display their plugins under all selected devices until they die out
-        client.query.device_id === 'unknown',
-    )
+  return Array.from(clients.values())
+    .filter((client: Client) => client.query.device_id === device.serial)
     .sort((a, b) => (a.query.app || '').localeCompare(b.query.app));
 }
 
-export function getClientByAppName(
-  clients: Client[],
+export function getClientsByAppName(
+  clients: Map<string, Client>,
   appName: string | null | undefined,
-): Client | undefined {
-  return clients.find((client) => client.query.app === appName);
+): Client[] {
+  return Array.from(clients.values()).filter(
+    (client) => client.query.app === appName,
+  );
 }
 
 export function getClientById(
@@ -586,3 +573,7 @@ export function isPluginEnabled(
   const enabledAppPlugins = enabledPlugins[appInfo.app];
   return enabledAppPlugins && enabledAppPlugins.indexOf(pluginId) > -1;
 }
+
+export function getAllClients(state: State): readonly Client[] {
+  return Array.from(state.clients.values());
+}

desktop/app/src/reducers/supportForm.tsx

@@ -116,13 +116,13 @@ export class Group {
     store.dispatch(
       setStaticView(require('../fb-stubs/SupportRequestFormV2').default),
     );
-    const selectedApp = store.getState().connections.selectedAppId;
-    const selectedClient = store.getState().connections.clients.find((o) => {
-      return o.id === store.getState().connections.selectedAppId;
-    });
+    const selectedAppId = store.getState().connections.selectedAppId;
+    const selectedClient = store
+      .getState()
+      .connections.clients.get(selectedAppId!);
     let errorMessage: string | undefined = undefined;
-    if (selectedApp) {
-      const {app} = deconstructClientId(selectedApp);
+    if (selectedAppId) {
+      const {app} = deconstructClientId(selectedAppId);
       const enabledPlugins: Array<string> | null =
         store.getState().connections.enabledPlugins[app];
       const unsupportedPlugins = [];

desktop/app/src/sandy-chrome/appinspect/AppSelector.tsx

@@ -22,7 +22,7 @@ import {batch} from 'react-redux';
 import {useDispatch, useStore} from '../../utils/useStore';
 import {
   canBeDefaultDevice,
-  getAvailableClients,
+  getClientsByDevice,
   selectClient,
   selectDevice,
 } from '../../reducers/connections';
@@ -85,7 +85,7 @@ export function AppSelector() {
     onSelectDevice,
     onSelectApp,
   );
-  const client = clients.find((client) => client.id === selectedAppId);
+  const client = clients.get(selectedAppId!);
 
   return (
     <>
@@ -194,7 +194,7 @@ const AppIconContainer = styled.div({
 
 function computeEntries(
   devices: BaseDevice[],
-  clients: Client[],
+  clients: Map<string, Client>,
   uninitializedClients: State['connections']['uninitializedClients'],
   onSelectDevice: (device: BaseDevice) => void,
   onSelectApp: (device: BaseDevice, client: Client) => void,
@@ -205,7 +205,7 @@ function computeEntries(
         // hide non default devices, unless they have a connected client or plugins
         canBeDefaultDevice(device) ||
         device.hasDevicePlugins ||
-        clients.some((c) => c.device === device),
+        getClientsByDevice(device, clients).length > 0,
     )
     .map((device) => {
       const deviceEntry = (
@@ -219,7 +219,7 @@ function computeEntries(
           <DeviceTitle device={device} />
         </Menu.Item>
       );
-      const clientEntries = getAvailableClients(device, clients).map(
+      const clientEntries = getClientsByDevice(device, clients).map(
         (client) => (
           <Menu.Item
             key={client.id}

desktop/app/src/sandy-chrome/appinspect/BookmarkSection.tsx

@@ -142,7 +142,7 @@ const NAVIGATION_PLUGIN_ID = 'Navigation';
 function navPluginStateSelector(state: State) {
   const {selectedAppId, clients} = state.connections;
   if (!selectedAppId) return undefined;
-  const client = clients.find((client) => client.id === selectedAppId);
+  const client = clients.get(selectedAppId);
   if (!client) return undefined;
   return client.sandyPluginStates.get(NAVIGATION_PLUGIN_ID)?.instanceApi as
     | undefined

desktop/app/src/sandy-chrome/notification/Notification.tsx

@@ -330,7 +330,7 @@ export function openNotification(store: Store, noti: PluginNotificationOrig) {
 }
 
 function getClientById(store: Store, identifier: string | null) {
-  return store.getState().connections.clients.find((c) => c.id === identifier);
+  return store.getState().connections.clients.get(identifier!);
 }
 
 function getDeviceById(store: Store, identifier: string | null) {

desktop/app/src/selectors/connections.tsx

@@ -16,19 +16,13 @@ import {
 import createSelector from './createSelector';
 
 const getSelectedPluginId = (state: State) => state.connections.selectedPlugin;
-const getSelectedAppId = (state: State) => state.connections.selectedAppId;
 const getSelectedDevice = (state: State) => state.connections.selectedDevice;
-const getClients = (state: State) => state.connections.clients;
 const getDevices = (state: State) => state.connections.devices;
 const getPluginDownloads = (state: State) => state.pluginDownloads;
 
-export const getActiveClient = createSelector(
-  getSelectedAppId,
-  getClients,
-  (selectedApp, clients) => {
-    return clients.find((c) => c.id === selectedApp) || null;
-  },
-);
+// N.B. no useSelector, It can't memoise on maps :-/
+export const getActiveClient = (state: State) =>
+  state.connections.clients.get(state.connections.selectedAppId!) ?? null;
 
 export const getMetroDevice = createSelector(getDevices, (devices) => {
   return (

desktop/app/src/utils/__tests__/exportData.node.tsx

@@ -33,7 +33,7 @@ import {
   sleep,
   Device,
 } from 'flipper-plugin';
-import {selectPlugin} from '../../reducers/connections';
+import {selectPlugin, getAllClients} from '../../reducers/connections';
 import {TestIdler} from '../Idler';
 import {TestDevice} from '../..';
 
@@ -1298,7 +1298,7 @@ test('Sandy plugins are imported properly', async () => {
 
   await importDataToStore('unittest.json', JSON.stringify(data), store);
 
-  const client2 = store.getState().connections.clients[1];
+  const client2 = getAllClients(store.getState().connections)[1];
   expect(client2).not.toBeFalsy();
   expect(client2).not.toBe(client);
   expect(Array.from(client2.plugins)).toEqual([TestPlugin.id]);
@@ -1511,15 +1511,13 @@ test('Sandy plugin with custom import', async () => {
   await importDataToStore('unittest.json', JSON.stringify(data), store);
 
   expect(
-    store
-      .getState()
-      .connections.clients[0].sandyPluginStates.get(plugin.id)
+    getAllClients(store.getState().connections)[0]
+      .sandyPluginStates.get(plugin.id)
       ?.instanceApi.counter.get(),
   ).toBe(0);
   expect(
-    store
-      .getState()
-      .connections.clients[1].sandyPluginStates.get(plugin.id)
+    getAllClients(store.getState().connections)[1]
+      .sandyPluginStates.get(plugin.id)
       ?.instanceApi.counter.get(),
   ).toBe(4);
 });
@@ -1635,9 +1633,9 @@ test('Sandy plugins with complex data are imported  / exported correctly', async
   ]);
 
   await importDataToStore('unittest.json', data.serializedString, store);
-  const api = store
-    .getState()
-    .connections.clients[1].sandyPluginStates.get(plugin.id)?.instanceApi;
+  const api = getAllClients(
+    store.getState().connections,
+  )[1].sandyPluginStates.get(plugin.id)?.instanceApi;
   expect(api.m.get()).toMatchInlineSnapshot(`
     Map {
       "a" => 1,

desktop/app/src/utils/createSandyPluginWrapper.tsx

@@ -194,7 +194,7 @@ export function createSandyPluginWrapper<S, A extends BaseAction, P>(
         (instance.device as BaseDevice)
       : // eslint-disable-next-line
           useStore((state) =>
-          state.connections.clients.find((c) => c.id === instance.appId),
+          state.connections.clients.get(instance.appId!),
         );
     if (!target) {
       throw new Error('Illegal state: missing target');

desktop/app/src/utils/exportData.tsx

@@ -420,7 +420,7 @@ async function getStoreExport(
   let state = store.getState();
   const {clients, selectedAppId, selectedDevice} = state.connections;
   const pluginsToProcess = determinePluginsToProcess(
-    clients,
+    Array.from(clients.values()),
     selectedDevice,
     state.plugins,
   );
@@ -433,7 +433,7 @@ async function getStoreExport(
   const fetchMetaDataMarker = `${EXPORT_FLIPPER_TRACE_EVENT}:fetch-meta-data`;
   performance.mark(fetchMetaDataMarker);
 
-  const client = clients.find((client) => client.id === selectedAppId);
+  const client = clients.get(selectedAppId!);
 
   const pluginStates2 = pluginsToProcess
     ? await exportSandyPluginStates(pluginsToProcess, idler, statusUpdate)