-
Notifications
You must be signed in to change notification settings - Fork 452
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
⚡️ Remove artificial 4ms nextTick()
delay when running in a browser
#647
Conversation
Fixes #646 ShareDB makes heavy use of [`nextTick()`][1]. It notably calls it every time we [send a message over a `StreamSocket`][2]. If ShareDB is running both `Backend` and `Client` in a browser (eg in client tests), `nextTick()` will [fall back to `setTimeout()`][3]. However, according to the [HTML standard][4]: > Timers can be nested; after five such nested timers, however, the > interval is forced to be at least four milliseconds. So using `setTimeout()` can incur a penalty of 4ms in the browser, just idling. Over the course of a test suite, which makes a lot of fast ShareDB requests in series, these delays can add up to many seconds or even minutes of idle CPU time. This change adds an alternative `nextTick()` implementation, using `MessageChannel`, which is present in both [Node.js][5] and [HTML][6] (with slightly different APIs, but close enough for our purposes). This class offers a way of waiting for a tick on the event loop, without the arbitrary 4ms delay. `MessageChannel` is [supported back to even IE10][7], and since Node.js v10.5.0, but if for some reason it's missing, we'll still fall back to `setTimeout()`. [1]: https://github.com/share/sharedb/blob/5259d0e0b66c50ff9e745fe34a55c5fb16c57a8e/lib/util.js#L88 [2]: https://github.com/share/sharedb/blob/5259d0e0b66c50ff9e745fe34a55c5fb16c57a8e/lib/stream-socket.js#L58 [3]: https://github.com/share/sharedb/blob/5259d0e0b66c50ff9e745fe34a55c5fb16c57a8e/lib/util.js#L98 [4]: https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#timers [5]: https://nodejs.org/api/worker_threads.html#class-messagechannel [6]: https://html.spec.whatwg.org/multipage/web-messaging.html#message-channels [7]: https://caniuse.com/mdn-api_messagechannel_messagechannel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To discuss: we can do this, but should we do it?
The 4ms is probably in the standard for some reason, but I don't really know what. Presumably the original goal was to prevent CPU starvation, but I don't know why you would decree an arbitrary 4ms, and not just say that the time can't be guaranteed (and then it should be up to the vendor to defer event loop tasks if it needs to do other browsery things like repaint, etc.).
I note that Safari isn't standards-compliant (shocker), and doesn't actually seem to enforce this minimum time, so clearly someone thinks it's not too important to wait for this time.
I acknowledge that my proposed use-case is for tests, and maybe shouldn't impact Production-grade code.
One potential option is to add a flag that can select a nextTick()
method? Or we just close this and I'll stick to my monkeypatch:
// Patch nextTick with a version that doesn't use setTimeout() so that our
// messages don't suffer artificial lag
shareDbUtils.nextTick = async function(callback: Function, ...args: any[]): Promise<void> {
await macrotask();
callback(...args);
};
function macrotask(): Promise<void> {
const channel = new MessageChannel();
return new Promise((resolve) => {
channel.port1.onmessage = () => {
resolve();
channel.port1.close();
};
channel.port2.postMessage('');
});
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also just realised that this PR won't actually help our situation 🤦🏼♂️ We have to include process
anyway (I think other Node.js polyfills might rely on it, like util
, which ShareDB also needs).
The only way this would help us is if we moved the MessageChannel
approach to be the first approach (above process.nextTick()
), but I can understand that might not be palatable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From discussion:
- There are a few places in browser code that call
util.nextTick
, but in normal usage none of them should nest - Existing bundled browser code should be unaffected since we currently require a polyfill of
util
, which pulls in aprocess
polyfill. - Might be worth separately updating
util.nextTick
to check on module init and swap implementations, instead of checking each call, though JS engines may mostly optimize that difference away
Fixes #646
ShareDB makes heavy use of
nextTick()
. It notably calls it every time we send a message over aStreamSocket
.If ShareDB is running both
Backend
andClient
in a browser (eg in client tests),nextTick()
will fall back tosetTimeout()
.However, according to the HTML standard:
So using
setTimeout()
can incur a penalty of 4ms in the browser, just idling.Over the course of a test suite, which makes a lot of fast ShareDB requests in series, these delays can add up to many seconds or even minutes of idle CPU time.
This change adds an alternative
nextTick()
implementation, usingMessageChannel
, which is present in both Node.js and HTML (with slightly different APIs, but close enough for our purposes). This class offers a way of waiting for a tick on the event loop, without the arbitrary 4ms delay.MessageChannel
is supported back to even IE10, and since Node.js v10.5.0, but if for some reason it's missing, we'll still fall back tosetTimeout()
.