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

process: code cleanup for nextTick #28047

Closed
wants to merge 5 commits into from
Closed

process: code cleanup for nextTick #28047

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1a66c0d
process: code cleanup for nextTick
apapirovski Jun 2, 2019
8c8c730
fixup: add destroy hook test
apapirovski Jun 5, 2019
c5c3c6f
fixup: add comment explaining the test
apapirovski Jun 5, 2019
20f2a93
fixup: make test work in workers
apapirovski Jun 7, 2019
83658f7
fixup: typo
apapirovski Jun 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions lib/internal/process/promises.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,17 +139,19 @@ function emitDeprecationWarning() {
}
}

// If this method returns true, at least one more tick need to be
// scheduled to process any potential pending rejections
// If this method returns true, we've executed user code or triggered
// a warning to be emitted which requires the microtask and next tick
// queues to be drained again.
function processPromiseRejections() {
let maybeScheduledTicksOrMicrotasks = asyncHandledRejections.length > 0;
Copy link
Member

Choose a reason for hiding this comment

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

On a side note, I wonder if it make sense now to just merge process/promises.js and process/task_queues.js - the latter is currently the only one who requires the former.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe? At the same time it's a little easier to read the task queue code as is. Also, the code in promises isn't really explicitly tied to task queues.

If someone feels strongly, they should open a PR. I, personally, like it as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Keep in mind, more files = more startup time.

Copy link
Member

@joyeecheung joyeecheung Jun 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@Fishrock123 This is now somewhat negligible since we have shipped pre-built code cache for builtin modules, which eliminates the majority of the overhead of loading an builtin module (compilation) - of course that does not apply to modules like internal/errors.js which does a lot of stuff in the initialization, but that overhead would be eliminated as well when the v8 snapshot integration is completed.


while (asyncHandledRejections.length > 0) {
const { promise, warning } = asyncHandledRejections.shift();
if (!process.emit('rejectionHandled', promise)) {
process.emitWarning(warning);
}
}

let maybeScheduledTicks = false;
let len = pendingUnhandledRejections.length;
while (len--) {
const promise = pendingUnhandledRejections.shift();
Expand All @@ -167,9 +169,10 @@ function processPromiseRejections() {
state === states.warn) {
emitWarning(uid, reason);
}
maybeScheduledTicks = true;
maybeScheduledTicksOrMicrotasks = true;
}
return maybeScheduledTicks || pendingUnhandledRejections.length !== 0;
return maybeScheduledTicksOrMicrotasks ||
pendingUnhandledRejections.length !== 0;
}

function getError(name, message) {
Expand Down
40 changes: 16 additions & 24 deletions lib/internal/process/task_queues.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,46 +65,38 @@ function processTicksAndRejections() {
while (tock = queue.shift()) {
const asyncId = tock[async_id_symbol];
emitBefore(asyncId, tock[trigger_async_id_symbol]);
// emitDestroy() places the async_id_symbol into an asynchronous queue
// that calls the destroy callback in the future. It's called before
// calling tock.callback so destroy will be called even if the callback
// throws an exception that is handled by 'uncaughtException' or a
// domain.
// TODO(trevnorris): This is a bit of a hack. It relies on the fact
// that nextTick() doesn't allow the event loop to proceed, but if
// any async hooks are enabled during the callback's execution then
// this tock's after hook will be called, but not its destroy hook.
if (destroyHooksExist())
emitDestroy(asyncId);

const callback = tock.callback;
if (tock.args === undefined)
callback();
else
callback(...tock.args);

try {
const callback = tock.callback;
if (tock.args === undefined)
callback();
else
callback(...tock.args);
} finally {
if (destroyHooksExist())
emitDestroy(asyncId);
}

emitAfter(asyncId);
}
setHasTickScheduled(false);
runMicrotasks();
} while (!queue.isEmpty() || processPromiseRejections());
setHasTickScheduled(false);
setHasRejectionToWarn(false);
}

class TickObject {
constructor(callback, args, triggerAsyncId) {
constructor(callback, args) {
this.callback = callback;
this.args = args;

const asyncId = newAsyncId();
const triggerAsyncId = getDefaultTriggerAsyncId();
this[async_id_symbol] = asyncId;
this[trigger_async_id_symbol] = triggerAsyncId;

if (initHooksExist()) {
emitInit(asyncId,
'TickObject',
triggerAsyncId,
this);
emitInit(asyncId, 'TickObject', triggerAsyncId, this);
}
}
}
Expand Down Expand Up @@ -132,7 +124,7 @@ function nextTick(callback) {

if (queue.isEmpty())
setHasTickScheduled(true);
queue.push(new TickObject(callback, args, getDefaultTriggerAsyncId()));
queue.push(new TickObject(callback, args));
}

let AsyncResource;
Expand Down