@samitouri / QOS-React / commits / 4ea12a11d1

[DevTools] Make Element Inspection Feel Snappy (#30555)

There's two problems. The biggest one is that it turns out that Chrome is throttling looping timers that we're using both while polling and for batching bridge traffic. This means that bridge traffic a lot of the time just slows down to 1 second at a time. No wonder it feels sluggish. The only solution is to not use timers for this. Even when it doesn't like in Firefox the batching into 100ms still feels too sluggish. The fix I use is to batch using a microtask instead so we can still batch multiple commands sent in a single event but we never artificially slow down an interaction. I don't think we've reevaluated this for a long time since this was in the initial commit of DevTools to this repo. If it causes other issues we can follow up on those. We really shouldn't use timers for debouncing and such. In fact, React itself recommends against it because we have a better technique with scheduling in Concurrent Mode. The correct way to implement this in the bridge is using a form of back-pressure where we don't keep sending messages until we get a message back and only send the last one that matters. E.g. when moving the cursor over a the elements tab we shouldn't let the backend one-by-one move the DOM node to each one we have ever passed. We should just move to the last one we're currently hovering over. But this can't be done at the bridge layer since it doesn't know if it's a last-one-wins or imperative operation where each one needs to be sent. It needs to be done higher. I'm not currently seeing any perf problems with this new approach but I'm curious on React Native or some thing. RN might need the back-pressure approach. That can be a follow up if we ever find a test case. Finally, the other problem is that we use a Suspense boundary around the Element Inspection. Suspense boundaries are for things that are expected to take a long time to load. This shows a loading state immediately. To avoid flashing when it ends up being fast, React throttles the reveal to 200ms. This means that we take a minimum of 200ms to show the props. The way to show fast async data in React is using a Transition (either using startTransition or useDeferredValue). This lets the old value remaining in place while we're loading the next one. We already implement this using `inspectedElementID` which is the async one. It would be more idiomatic to implement this with useDeferredValue rather than the reducer we have now but same principle. We were just using the wrong ID in a few places so when it synchronously updated they suspended. So I just made them use the inspectedElementID instead. Then I can simply remove the Suspense boundary. Now the selection updates in the tree view synchronously and the sidebar lags a frame or two but it feels instant. It doesn't flash to white between which is key.

Sebastian Markbåge committed Aug 1, 2024 at 11:04 UTC 4ea12a11d1848c1398f9a8babcfbcd51e150f1d9
6 files changed +44 -53
packages/react-devtools-shared/src/__tests__/inspectedElement-test.js
+5 -6
@@ -117,12 +117,11 @@ describe('InspectedElement', () => {
117 <SettingsContextController>
118 <TreeContextController
119 defaultSelectedElementID={defaultSelectedElementID}
120 - defaultSelectedElementIndex={defaultSelectedElementIndex}>
121 - <React.Suspense fallback="Loading...">
122 - <InspectedElementContextController>
123 - {children}
124 - </InspectedElementContextController>
125 - </React.Suspense>
120 + defaultSelectedElementIndex={defaultSelectedElementIndex}
121 + defaultInspectedElementID={defaultSelectedElementID}>
122 + <InspectedElementContextController>
123 + {children}
124 + </InspectedElementContextController>
125 </TreeContextController>
126 </SettingsContextController>
127 </StoreContext.Provider>
packages/react-devtools-shared/src/__tests__/setupTests.js
+5
@@ -128,6 +128,11 @@ beforeEach(() => {
128 // Fake timers let us flush Bridge operations between setup and assertions.
129 jest.useFakeTimers();
130
131 + // We use fake timers heavily in tests but the bridge batching now uses microtasks.
132 + global.devtoolsJestTestScheduler = callback => {
133 + setTimeout(callback, 0);
134 + };
135 +
136 // Use utils.js#withErrorsOrWarningsIgnored instead of directly mutating this array.
137 global._ignoredErrorOrWarningMessages = [
138 'react-test-renderer is deprecated.',
packages/react-devtools-shared/src/bridge.js
+24 -26
@@ -19,8 +19,6 @@ import type {
19 } from 'react-devtools-shared/src/backend/types';
20 import type {StyleAndLayout as StyleAndLayoutPayload} from 'react-devtools-shared/src/backend/NativeStyleEditor/types';
21
22 -const BATCH_DURATION = 100;
23 -
22 // This message specifies the version of the DevTools protocol currently supported by the backend,
23 // as well as the earliest NPM version (e.g. "4.13.0") that protocol is supported by on the frontend.
24 // This enables an older frontend to display an upgrade message to users for a newer, unsupported backend.
@@ -276,7 +274,7 @@ class Bridge<
274 }> {
275 _isShutdown: boolean = false;
276 _messageQueue: Array<any> = [];
279 - _timeoutID: TimeoutID | null = null;
277 + _scheduledFlush: boolean = false;
278 _wall: Wall;
279 _wallUnlisten: Function | null = null;
280
@@ -324,8 +322,19 @@ class Bridge<
322 // (or we're waiting for our setTimeout-0 to fire), then _timeoutID will
323 // be set, and we'll simply add to the queue and wait for that
324 this._messageQueue.push(event, payload);
327 - if (!this._timeoutID) {
328 - this._timeoutID = setTimeout(this._flush, 0);
325 + if (!this._scheduledFlush) {
326 + this._scheduledFlush = true;
327 + // $FlowFixMe
328 + if (typeof devtoolsJestTestScheduler === 'function') {
329 + // This exists just for our own jest tests.
330 + // They're written in such a way that we can neither mock queueMicrotask
331 + // because then we break React DOM and we can't not mock it because then
332 + // we can't synchronously flush it. So they need to be rewritten.
333 + // $FlowFixMe
334 + devtoolsJestTestScheduler(this._flush); // eslint-disable-line no-undef
335 + } else {
336 + queueMicrotask(this._flush);
337 + }
338 }
339 }
340
@@ -363,34 +372,23 @@ class Bridge<
372 do {
373 this._flush();
374 } while (this._messageQueue.length);
366 -
367 - // Make sure once again that there is no dangling timer.
368 - if (this._timeoutID !== null) {
369 - clearTimeout(this._timeoutID);
370 - this._timeoutID = null;
371 - }
375 }
376
377 _flush: () => void = () => {
378 // This method is used after the bridge is marked as destroyed in shutdown sequence,
379 // so we do not bail out if the bridge marked as destroyed.
380 // It is a private method that the bridge ensures is only called at the right times.
378 -
379 - if (this._timeoutID !== null) {
380 - clearTimeout(this._timeoutID);
381 - this._timeoutID = null;
382 - }
383 -
384 - if (this._messageQueue.length) {
385 - for (let i = 0; i < this._messageQueue.length; i += 2) {
386 - this._wall.send(this._messageQueue[i], ...this._messageQueue[i + 1]);
381 + try {
382 + if (this._messageQueue.length) {
383 + for (let i = 0; i < this._messageQueue.length; i += 2) {
384 + this._wall.send(this._messageQueue[i], ...this._messageQueue[i + 1]);
385 + }
386 + this._messageQueue.length = 0;
387 }
388 - this._messageQueue.length = 0;
389 -
390 - // Check again for queued messages in BATCH_DURATION ms. This will keep
391 - // flushing in a loop as long as messages continue to be added. Once no
392 - // more are, the timer expires.
393 - this._timeoutID = setTimeout(this._flush, BATCH_DURATION);
388 + } finally {
389 + // We set this at the end in case new messages are added synchronously above.
390 + // They're already handled so they shouldn't queue more flushes.
391 + this._scheduledFlush = false;
392 }
393 };
394
packages/react-devtools-shared/src/devtools/views/Components/Components.js
+4 -17
@@ -8,14 +8,7 @@
8 */
9
10 import * as React from 'react';
11 -import {
12 - Fragment,
13 - Suspense,
14 - useEffect,
15 - useLayoutEffect,
16 - useReducer,
17 - useRef,
18 -} from 'react';
11 +import {Fragment, useEffect, useLayoutEffect, useReducer, useRef} from 'react';
12 import Tree from './Tree';
13 import {OwnersListContextController} from './OwnersListContext';
14 import portaledContent from '../portaledContent';
@@ -169,11 +162,9 @@ function Components(_: {}) {
162 <div className={styles.InspectedElementWrapper}>
163 <NativeStyleContextController>
164 <InspectedElementErrorBoundary>
172 - <Suspense fallback={<Loading />}>
173 - <InspectedElementContextController>
174 - <InspectedElement />
175 - </InspectedElementContextController>
176 - </Suspense>
165 + <InspectedElementContextController>
166 + <InspectedElement />
167 + </InspectedElementContextController>
168 </InspectedElementErrorBoundary>
169 </NativeStyleContextController>
170 </div>
@@ -186,10 +177,6 @@ function Components(_: {}) {
177 );
178 }
179
189 -function Loading() {
190 - return <div className={styles.Loading}>Loading...</div>;
191 -}
192 -
180 const LOCAL_STORAGE_KEY = 'React::DevTools::createResizeReducer';
181 const VERTICAL_MODE_MAX_WIDTH = 600;
182 const MINIMUM_SIZE = 50;
packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContext.js
+4 -2
@@ -66,7 +66,7 @@ export type Props = {
66 export function InspectedElementContextController({
67 children,
68 }: Props): React.Node {
69 - const {selectedElementID} = useContext(TreeStateContext);
69 + const {inspectedElementID} = useContext(TreeStateContext);
70 const fetchFileWithCaching = useContext(FetchFileWithCachingContext);
71 const bridge = useContext(BridgeContext);
72 const store = useContext(StoreContext);
@@ -93,7 +93,9 @@ export function InspectedElementContextController({
93 });
94
95 const element =
96 - selectedElementID !== null ? store.getElementByID(selectedElementID) : null;
96 + inspectedElementID !== null
97 + ? store.getElementByID(inspectedElementID)
98 + : null;
99
100 const alreadyLoadedHookNames =
101 element != null && hasAlreadyLoadedHookNames(element);
packages/react-devtools-shared/src/devtools/views/Components/InspectedElementErrorBoundary.js
+2 -2
@@ -27,7 +27,7 @@ export default function InspectedElementErrorBoundaryWrapper({
27 }: WrapperProps): React.Node {
28 // Key on the selected element ID so that changing the selected element automatically hides the boundary.
29 // This seems best since an error inspecting one element isn't likely to be relevant to another element.
30 - const {selectedElementID} = useContext(TreeStateContext);
30 + const {inspectedElementID} = useContext(TreeStateContext);
31
32 const refresh = useCacheRefresh();
33 const handleDsmiss = useCallback(() => {
@@ -37,7 +37,7 @@ export default function InspectedElementErrorBoundaryWrapper({
37 return (
38 <div className={styles.Wrapper}>
39 <ErrorBoundary
40 - key={selectedElementID}
40 + key={inspectedElementID}
41 canDismiss={true}
42 onBeforeDismissCallback={handleDsmiss}>
43 {children}