Skip to content

Commit

Permalink
Use queueMicrotask to batch bridge messages
Browse files Browse the repository at this point in the history
This timeout is too long. It makes the UI feels sluggish.

Chrome also throttles looping timers aggressively which makes it worse.

We also shouldn't rely on the throttling at this level. It doesn't help
when you spam the receiving side with thousands of messages to process
anyway. Instead we need to implement a form of backpressure to avoid sending
so much in the first place.
  • Loading branch information
sebmarkbage committed Aug 1, 2024
1 parent 8ee7311 commit c55b626
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 26 deletions.
5 changes: 5 additions & 0 deletions packages/react-devtools-shared/src/__tests__/setupTests.js
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,11 @@ beforeEach(() => {
// Fake timers let us flush Bridge operations between setup and assertions.
jest.useFakeTimers();

// We use fake timers heavily in tests but the bridge batching now uses microtasks.
global.devtoolsJestTestScheduler = callback => {
setTimeout(callback, 0);
};

// Use utils.js#withErrorsOrWarningsIgnored instead of directly mutating this array.
global._ignoredErrorOrWarningMessages = [
'react-test-renderer is deprecated.',
Expand Down
50 changes: 24 additions & 26 deletions packages/react-devtools-shared/src/bridge.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@ import type {
} from 'react-devtools-shared/src/backend/types';
import type {StyleAndLayout as StyleAndLayoutPayload} from 'react-devtools-shared/src/backend/NativeStyleEditor/types';

const BATCH_DURATION = 100;

// This message specifies the version of the DevTools protocol currently supported by the backend,
// as well as the earliest NPM version (e.g. "4.13.0") that protocol is supported by on the frontend.
// This enables an older frontend to display an upgrade message to users for a newer, unsupported backend.
Expand Down Expand Up @@ -276,7 +274,7 @@ class Bridge<
}> {
_isShutdown: boolean = false;
_messageQueue: Array<any> = [];
_timeoutID: TimeoutID | null = null;
_scheduledFlush: boolean = false;
_wall: Wall;
_wallUnlisten: Function | null = null;

Expand Down Expand Up @@ -324,8 +322,19 @@ class Bridge<
// (or we're waiting for our setTimeout-0 to fire), then _timeoutID will
// be set, and we'll simply add to the queue and wait for that
this._messageQueue.push(event, payload);
if (!this._timeoutID) {
this._timeoutID = setTimeout(this._flush, 0);
if (!this._scheduledFlush) {
this._scheduledFlush = true;
// $FlowFixMe
if (typeof devtoolsJestTestScheduler === 'function') {
// This exists just for our own jest tests.
// They're written in such a way that we can neither mock queueMicrotask
// because then we break React DOM and we can't not mock it because then
// we can't synchronously flush it. So they need to be rewritten.
// $FlowFixMe
devtoolsJestTestScheduler(this._flush); // eslint-disable-line no-undef
} else {
queueMicrotask(this._flush);
}
}
}

Expand Down Expand Up @@ -363,34 +372,23 @@ class Bridge<
do {
this._flush();
} while (this._messageQueue.length);

// Make sure once again that there is no dangling timer.
if (this._timeoutID !== null) {
clearTimeout(this._timeoutID);
this._timeoutID = null;
}
}

_flush: () => void = () => {
// This method is used after the bridge is marked as destroyed in shutdown sequence,
// so we do not bail out if the bridge marked as destroyed.
// It is a private method that the bridge ensures is only called at the right times.

if (this._timeoutID !== null) {
clearTimeout(this._timeoutID);
this._timeoutID = null;
}

if (this._messageQueue.length) {
for (let i = 0; i < this._messageQueue.length; i += 2) {
this._wall.send(this._messageQueue[i], ...this._messageQueue[i + 1]);
try {
if (this._messageQueue.length) {
for (let i = 0; i < this._messageQueue.length; i += 2) {
this._wall.send(this._messageQueue[i], ...this._messageQueue[i + 1]);
}
this._messageQueue.length = 0;
}
this._messageQueue.length = 0;

// Check again for queued messages in BATCH_DURATION ms. This will keep
// flushing in a loop as long as messages continue to be added. Once no
// more are, the timer expires.
this._timeoutID = setTimeout(this._flush, BATCH_DURATION);
} finally {
// We set this at the end in case new messages are added synchronously above.
// They're already handled so they shouldn't queue more flushes.
this._scheduledFlush = false;
}
};

Expand Down

0 comments on commit c55b626

Please sign in to comment.