From 2e5b52d247c366b1ea7e4acce8c5444e7f46da6f Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Wed, 18 Nov 2020 08:49:30 -0800 Subject: [PATCH] batch for more efficient message processing Summary: `unstablebatched_updates` should be used whenever a non-react originating event might affect multiple components, to make sure that React batches them optimally. Applied it to the most import events that handle incoming device events Reviewed By: nikoant Differential Revision: D25052937 fbshipit-source-id: b2c783fb9c43be371553db39969280f9d7c3e260 --- desktop/app/src/Client.tsx | 208 +++++++++--------- desktop/app/src/utils/messageQueue.tsx | 69 +++--- .../flipper-plugin/src/__tests__/api.node.tsx | 1 + desktop/flipper-plugin/src/index.ts | 1 + desktop/flipper-plugin/src/plugin/Plugin.tsx | 13 +- .../flipper-plugin/src/plugin/PluginBase.tsx | 13 +- desktop/flipper-plugin/src/state/batch.tsx | 24 ++ 7 files changed, 182 insertions(+), 147 deletions(-) create mode 100644 desktop/flipper-plugin/src/state/batch.tsx diff --git a/desktop/app/src/Client.tsx b/desktop/app/src/Client.tsx index f207a8def..dbe21d902 100644 --- a/desktop/app/src/Client.tsx +++ b/desktop/app/src/Client.tsx @@ -391,111 +391,113 @@ export default class Client extends EventEmitter { return; } - let rawData; - try { - rawData = JSON.parse(msg); - } catch (err) { - console.error(`Invalid JSON: ${msg}`, 'clientMessage'); - return; - } - - const data: { - id?: number; - method?: string; - params?: Params; - success?: Object; - error?: ErrorType; - } = rawData; - - const {id, method} = data; - - if ( - data.params?.api != 'flipper-messages' && - flipperMessagesClientPlugin.isConnected() - ) { - flipperMessagesClientPlugin.newMessage({ - device: this.deviceSync?.displayTitle(), - app: this.query.app, - flipperInternalMethod: method, - plugin: data.params?.api, - pluginMethod: data.params?.method, - payload: data.params?.params, - direction: 'toFlipper:message', - }); - } - - if (id == null) { - const {error} = data; - if (error != null) { - console.error( - `Error received from device ${ - method ? `when calling ${method}` : '' - }: ${error.message} + \nDevice Stack Trace: ${error.stacktrace}`, - 'deviceError', - ); - handleError(this.store, this.deviceSync, error); - } else if (method === 'refreshPlugins') { - this.refreshPlugins(); - } else if (method === 'execute') { - invariant(data.params, 'expected params'); - const params: Params = data.params; - const bytes = msg.length * 2; // string lengths are measured in UTF-16 units (not characters), so 2 bytes per char - emitBytesReceived(params.api, bytes); - - const persistingPlugin: PluginDefinition | undefined = - this.store.getState().plugins.clientPlugins.get(params.api) || - this.store.getState().plugins.devicePlugins.get(params.api); - - let handled = false; // This is just for analysis - if ( - persistingPlugin && - ((persistingPlugin as any).persistedStateReducer || - // only send messages to enabled sandy plugins - this.sandyPluginStates.has(params.api)) - ) { - handled = true; - const pluginKey = getPluginKey( - this.id, - {serial: this.query.device_id}, - params.api, - ); - if (!this.messageBuffer[pluginKey]) { - this.messageBuffer[pluginKey] = { - plugin: (this.sandyPluginStates.get(params.api) ?? - persistingPlugin) as any, - messages: [params], - }; - } else { - this.messageBuffer[pluginKey].messages.push(params); - } - this.flushMessageBufferDebounced(); - } - const apiCallbacks = this.broadcastCallbacks.get(params.api); - if (apiCallbacks) { - const methodCallbacks = apiCallbacks.get(params.method); - if (methodCallbacks) { - for (const callback of methodCallbacks) { - handled = true; - callback(params.params); - } - } - } - if (!handled) { - console.warn(`Unhandled message ${params.api}.${params.method}`); - } - } - return; // method === 'execute' - } - - if (this.sdkVersion < 1) { - const callbacks = this.requestCallbacks.get(id); - if (!callbacks) { + batch(() => { + let rawData; + try { + rawData = JSON.parse(msg); + } catch (err) { + console.error(`Invalid JSON: ${msg}`, 'clientMessage'); return; } - this.requestCallbacks.delete(id); - this.finishTimingRequestResponse(callbacks.metadata); - this.onResponse(data, callbacks.resolve, callbacks.reject); - } + + const data: { + id?: number; + method?: string; + params?: Params; + success?: Object; + error?: ErrorType; + } = rawData; + + const {id, method} = data; + + if ( + data.params?.api != 'flipper-messages' && + flipperMessagesClientPlugin.isConnected() + ) { + flipperMessagesClientPlugin.newMessage({ + device: this.deviceSync?.displayTitle(), + app: this.query.app, + flipperInternalMethod: method, + plugin: data.params?.api, + pluginMethod: data.params?.method, + payload: data.params?.params, + direction: 'toFlipper:message', + }); + } + + if (id == null) { + const {error} = data; + if (error != null) { + console.error( + `Error received from device ${ + method ? `when calling ${method}` : '' + }: ${error.message} + \nDevice Stack Trace: ${error.stacktrace}`, + 'deviceError', + ); + handleError(this.store, this.deviceSync, error); + } else if (method === 'refreshPlugins') { + this.refreshPlugins(); + } else if (method === 'execute') { + invariant(data.params, 'expected params'); + const params: Params = data.params; + const bytes = msg.length * 2; // string lengths are measured in UTF-16 units (not characters), so 2 bytes per char + emitBytesReceived(params.api, bytes); + + const persistingPlugin: PluginDefinition | undefined = + this.store.getState().plugins.clientPlugins.get(params.api) || + this.store.getState().plugins.devicePlugins.get(params.api); + + let handled = false; // This is just for analysis + if ( + persistingPlugin && + ((persistingPlugin as any).persistedStateReducer || + // only send messages to enabled sandy plugins + this.sandyPluginStates.has(params.api)) + ) { + handled = true; + const pluginKey = getPluginKey( + this.id, + {serial: this.query.device_id}, + params.api, + ); + if (!this.messageBuffer[pluginKey]) { + this.messageBuffer[pluginKey] = { + plugin: (this.sandyPluginStates.get(params.api) ?? + persistingPlugin) as any, + messages: [params], + }; + } else { + this.messageBuffer[pluginKey].messages.push(params); + } + this.flushMessageBufferDebounced(); + } + const apiCallbacks = this.broadcastCallbacks.get(params.api); + if (apiCallbacks) { + const methodCallbacks = apiCallbacks.get(params.method); + if (methodCallbacks) { + for (const callback of methodCallbacks) { + handled = true; + callback(params.params); + } + } + } + if (!handled) { + console.warn(`Unhandled message ${params.api}.${params.method}`); + } + } + return; // method === 'execute' + } + + if (this.sdkVersion < 1) { + const callbacks = this.requestCallbacks.get(id); + if (!callbacks) { + return; + } + this.requestCallbacks.delete(id); + this.finishTimingRequestResponse(callbacks.metadata); + this.onResponse(data, callbacks.resolve, callbacks.reject); + } + }); } onResponse( diff --git a/desktop/app/src/utils/messageQueue.tsx b/desktop/app/src/utils/messageQueue.tsx index b15a33f4a..18a629227 100644 --- a/desktop/app/src/utils/messageQueue.tsx +++ b/desktop/app/src/utils/messageQueue.tsx @@ -24,7 +24,7 @@ import {Idler, BaseIdler} from './Idler'; import {pluginIsStarred, getSelectedPluginKey} from '../reducers/connections'; import {deconstructPluginKey} from './clientUtils'; import {defaultEnabledBackgroundPlugins} from './pluginUtils'; -import {_SandyPluginInstance} from 'flipper-plugin'; +import {batch, _SandyPluginInstance} from 'flipper-plugin'; import {addBackgroundStat} from './pluginStats'; function processMessageClassic( @@ -187,39 +187,44 @@ export async function processMessageQueue( : getCurrentPluginState(store, plugin, pluginKey); let offset = 0; let newPluginState = persistedState; - do { - if (_SandyPluginInstance.is(plugin)) { - // Optimization: we could send a batch of messages here - processMessagesSandy(pluginKey, plugin, [messages[offset]]); - } else { - newPluginState = processMessageClassic( - newPluginState, - pluginKey, - plugin, - messages[offset], + batch(() => { + do { + if (_SandyPluginInstance.is(plugin)) { + // Optimization: we could send a batch of messages here + processMessagesSandy(pluginKey, plugin, [messages[offset]]); + } else { + newPluginState = processMessageClassic( + newPluginState, + pluginKey, + plugin, + messages[offset], + ); + } + offset++; + progress++; + + progressCallback?.({ + total: Math.max(total, progress), + current: progress, + }); + } while (offset < messages.length && !idler.shouldIdle()); + // save progress + // by writing progress away first and then idling, we make sure this logic is + // resistent to kicking off this process twice; grabbing, processing messages, saving state is done synchronosly + // until the idler has to break + store.dispatch(clearMessageQueue(pluginKey, offset)); + if ( + !_SandyPluginInstance.is(plugin) && + newPluginState !== persistedState + ) { + store.dispatch( + setPluginState({ + pluginKey, + state: newPluginState, + }), ); } - offset++; - progress++; - - progressCallback?.({ - total: Math.max(total, progress), - current: progress, - }); - } while (offset < messages.length && !idler.shouldIdle()); - // save progress - // by writing progress away first and then idling, we make sure this logic is - // resistent to kicking off this process twice; grabbing, processing messages, saving state is done synchronosly - // until the idler has to break - store.dispatch(clearMessageQueue(pluginKey, offset)); - if (!_SandyPluginInstance.is(plugin) && newPluginState !== persistedState) { - store.dispatch( - setPluginState({ - pluginKey, - state: newPluginState, - }), - ); - } + }); if (idler.isCancelled()) { return false; diff --git a/desktop/flipper-plugin/src/__tests__/api.node.tsx b/desktop/flipper-plugin/src/__tests__/api.node.tsx index 45deb9e96..68258640c 100644 --- a/desktop/flipper-plugin/src/__tests__/api.node.tsx +++ b/desktop/flipper-plugin/src/__tests__/api.node.tsx @@ -29,6 +29,7 @@ test('Correct top level API exposed', () => { "Layout", "NUX", "TestUtils", + "batch", "createState", "renderReactRoot", "theme", diff --git a/desktop/flipper-plugin/src/index.ts b/desktop/flipper-plugin/src/index.ts index f960141ec..6b617602f 100644 --- a/desktop/flipper-plugin/src/index.ts +++ b/desktop/flipper-plugin/src/index.ts @@ -30,6 +30,7 @@ export { usePlugin, } from './plugin/PluginContext'; export {createState, useValue, Atom} from './state/atom'; +export {batch} from './state/batch'; export {FlipperLib} from './plugin/FlipperLib'; export { MenuEntry, diff --git a/desktop/flipper-plugin/src/plugin/Plugin.tsx b/desktop/flipper-plugin/src/plugin/Plugin.tsx index 2b3d1ac97..18bb293c8 100644 --- a/desktop/flipper-plugin/src/plugin/Plugin.tsx +++ b/desktop/flipper-plugin/src/plugin/Plugin.tsx @@ -11,6 +11,7 @@ import {SandyPluginDefinition} from './SandyPluginDefinition'; import {BasePluginInstance, BasePluginClient} from './PluginBase'; import {FlipperLib} from './FlipperLib'; import {RealFlipperDevice} from './DevicePlugin'; +import {batched} from '../state/batch'; type EventsContract = Record; type MethodsContract = Record Promise>; @@ -146,10 +147,10 @@ export class SandyPluginInstance extends BasePluginInstance { return realClient.query.app; }, onConnect: (cb) => { - this.events.on('connect', cb); + this.events.on('connect', batched(cb)); }, onDisconnect: (cb) => { - this.events.on('disconnect', cb); + this.events.on('disconnect', batched(cb)); }, send: async (method, params) => { this.assertConnected(); @@ -160,11 +161,11 @@ export class SandyPluginInstance extends BasePluginInstance { params as any, ); }, - onMessage: (event, callback) => { - this.events.on('event-' + event, callback); + onMessage: (event, cb) => { + this.events.on('event-' + event, batched(cb)); }, - onUnhandledMessage: (callback) => { - this.events.on('unhandled-event', callback); + onUnhandledMessage: (cb) => { + this.events.on('unhandled-event', batched(cb)); }, supportsMethod: async (method) => { this.assertConnected(); diff --git a/desktop/flipper-plugin/src/plugin/PluginBase.tsx b/desktop/flipper-plugin/src/plugin/PluginBase.tsx index b632d4e39..15e6f3723 100644 --- a/desktop/flipper-plugin/src/plugin/PluginBase.tsx +++ b/desktop/flipper-plugin/src/plugin/PluginBase.tsx @@ -13,6 +13,7 @@ import {Atom} from '../state/atom'; import {MenuEntry, NormalizedMenuEntry, normalizeMenuEntry} from './MenuEntry'; import {FlipperLib} from './FlipperLib'; import {Device, RealFlipperDevice} from './DevicePlugin'; +import {batched} from '../state/batch'; export interface BasePluginClient { readonly device: Device; @@ -116,7 +117,7 @@ export abstract class BasePluginInstance { // To be called from constructory setCurrentPluginInstance(this); try { - this.instanceApi = factory(); + this.instanceApi = batched(factory)(); } finally { this.initialStates = undefined; setCurrentPluginInstance(undefined); @@ -127,16 +128,16 @@ export abstract class BasePluginInstance { return { device: this.device, onActivate: (cb) => { - this.events.on('activate', cb); + this.events.on('activate', batched(cb)); }, onDeactivate: (cb) => { - this.events.on('deactivate', cb); + this.events.on('deactivate', batched(cb)); }, - onDeepLink: (callback) => { - this.events.on('deeplink', callback); + onDeepLink: (cb) => { + this.events.on('deeplink', batched(cb)); }, onDestroy: (cb) => { - this.events.on('destroy', cb); + this.events.on('destroy', batched(cb)); }, addMenuEntry: (...entries) => { for (const entry of entries) { diff --git a/desktop/flipper-plugin/src/state/batch.tsx b/desktop/flipper-plugin/src/state/batch.tsx new file mode 100644 index 000000000..34984f127 --- /dev/null +++ b/desktop/flipper-plugin/src/state/batch.tsx @@ -0,0 +1,24 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @format + */ + +import {unstable_batchedUpdates} from 'react-dom'; + +export const batch = unstable_batchedUpdates; + +export function batched(fn: T): T; +export function batched(fn: any) { + return function (this: any) { + let res: any; + batch(() => { + // eslint-disable-next-line + res = fn.apply(this, arguments); + }); + return res; + }; +}