Skip to content
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.

Sign up for GitHub

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

Jump to bottom

optimization timers promisify #9485

Closed
wants to merge 2 commits into from
Closed

optimization timers promisify #9485

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d5e7588
optimization
sirenkovladd Mar 18, 2024
e2ed11e
small optimization
sirenkovladd Mar 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 8 additions & 6 deletions src/js/node/util.js
View file
Open in desktop
Expand Up @@ -166,24 +166,26 @@ function defineCustomPromisify(target, callback) {
{
const { setTimeout: timeout, setImmediate: immediate, setInterval: interval } = globalThis;

let timersPromises;

if (timeout) {
defineCustomPromisify(timeout, function setTimeout(arg1) {
const fn = defineCustomPromisify(timeout, require("node:timers/promises").setTimeout);
return fn.$apply(this, arguments);
if (!timersPromises) timersPromises = require("node:timers/promises");
return timersPromises.setTimeout.$apply(this, arguments);
});
}

if (immediate) {
defineCustomPromisify(immediate, function setImmediate(arg1) {
const fn = defineCustomPromisify(immediate, require("node:timers/promises").setImmediate);
return fn.$apply(this, arguments);
if (!timersPromises) timersPromises = require("node:timers/promises");
return timersPromises.setImmediate.$apply(this, arguments);
});
}

if (interval) {
defineCustomPromisify(interval, function setInterval(arg1) {
const fn = defineCustomPromisify(interval, require("node:timers/promises").setInterval);
return fn.$apply(this, arguments);
if (!timersPromises) timersPromises = require("node:timers/promises");
return timersPromises.setInterval.$apply(this, arguments);
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this @sirenkovladd! Can you provide a little microbenchmark result. We have gone back and forth on this pattern and a microbenchmark would help solidify a path.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the following optimization pattern should not affect performance. unlike in user code, require is inlined to the following code:

(@getInternalField(@internalModuleRegistry, 21/*node:events*/) || @createInternalModuleById(21/*node:events*/));

(this transform is done in internal-module-registry-scanner.ts, called in sliceSourceCode)

the first half of this is already a cache lookup, specifically a native array access into the JSInternalFieldObjectImpl in InternalModuleRegistry.h/cpp. this lookup should be incredibly instant and very JIT friendly. reminder that getInternalField is a bytecode intrinsic (see emit_intrinsic_getInternalField). then we check if it is not defined and take the || path which will call into a native function @createInternalModuleById (see jsCreateInternalModuleById). the only thing that is slow is the latter call, which is already guarded by a conditional expression.

my stance on this pattern is that it is a unnecessary optimization, and that we should actually reverse any places that we do this.

however ... i have not benchmarked the above, and i'm open to being proved wrong.

but then again, i dont think there is any application that the initialization of util.promisify on the timer becomes the slow point of an application.

}
Expand Down