async_hooks: minor refactor to callback invocation #13419

addaleax · 2017-06-02T22:19:57Z

Re-use the init function wherever possible, and move
try { … } catch blocks that result in fatal errors to a larger
scope.

Also make the argument order for init() consistent in the codebase.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

async_hooks /cc @nodejs/async_hooks

refack

Very nice

refack · 2017-06-02T22:28:27Z

lib/async_hooks.js

-    if (typeof active_hooks_array[i][before_symbol] === 'function') {
-      runCallback(active_hooks_array[i][before_symbol], asyncId);
+  try {
+    for (var i = 0; i < active_hooks_array.length; i++) {


Why not for ... of?

I don’t know, but I wouldn’t be surprised if this variant would continue to be faster (i.e. I’m reluctant to change this without benchmarking – if you do and you get good results, sure).

The original reason for this was that the array index was used. This was before the design decision to not allow the callable hooks to be altered during execution of all hooks; not for benchmarking reasons. Though I'd say no sense in changing it unless benchmarks show there is an improvement.

Ack.
Was curious so I just tested and FOI (For Our Information):

> var a = [1] undefined > for (let b of a) { b === 1 && a.push(2); console.log(b)} 1 2

@refack That's no longer an issue, which is why the index is no longer used. At the top of the file you'll see:

// Use to temporarily store and updated active_hooks_array if the user enables // or disables a hook while hooks are being processed. var tmp_active_hooks_array = null; // Keep track of the field counts held in tmp_active_hooks_array. var tmp_async_hook_fields = null;

Which getHooksArray() automatically assigns the old array to and duplicates the new altered array. So it's impossible for the user to alter the hooks mid-execution. i.e. you'll never see the case of "a.push()" to the actual array that contains the hooks in the middle of execution. So feel free to benchmark it all you want.

AndreasMadsen · 2017-06-02T23:58:29Z

lib/async_hooks.js

  emitBeforeN(asyncId);
 }


 // Called from native. The asyncId stack handling is taken care of there before
 // this is called.
 function emitAfterN(asyncId) {
-  if (async_hook_fields[kAfter] > 0) {
-    processing_hook = true;
+  processing_hook = true;


Why was the if (async_hook_fields[kAfter] > 0) short circuit removed?

I’ve added it to emitAfterS now; The idea is that the native code doesn’t bother calling emitAfterN if the field is 0, so checking it here is pointless for the common case of a call from C++.

Note for future change: I'd like to include a macros.py similar to deps/v8/src/js/macros.py for Debug builds. except it replaces comments instead of normal expressions.

In the code you'll find // CHECK() comments that were meant as reminders of important bits to check, but not important enough to check in a Release build. Though the difference would be that

// CHECK(expr)

is replaced with something like

assert.ok(expr);

But only for debug builds.

Point here would be to include the following here:

// CHECK(async_hook_fields[kAfter] > 0)

as a sanity check for the future in case a change is made that invalidates your comment of

The idea is that the native code doesn’t bother calling emitAfterN if the field is 0, so checking it here is pointless for the common case of a call from C++.

refack · 2017-06-03T05:31:06Z

lib/async_hooks.js

@@ -412,11 +397,17 @@ function emitDestroyS(asyncId) {


 function emitDestroyN(asyncId) {
+  if (async_hook_fields[kDestroy] === 0) return;


Re: https://github.com/nodejs/node/pull/13419/files#r119971593 why was this added?

Right, this is superfluous, thanks for pointing out.

addaleax · 2017-06-03T09:45:38Z

@refack @AndreasMadsen Thanks for pointing these out, this is updated now

AndreasMadsen

LGTM

trevnorris · 2017-06-06T22:31:49Z

@addaleax Mind adding a comment either in the code or commit message body about why moving the try/catch outside the for loop is an improvement? I have no issue w/ it. Just for posterity.

trevnorris

Left a comment about leaving a more detailed comment, but not a blocker. LGTM

Re-use the `init` function wherever possible, and move `try { … } catch` blocks that result in fatal errors to a larger scope. Also make the argument order for `init()` consistent in the codebase.

addaleax · 2017-06-07T19:15:16Z

Rebased and added a comment about the try/catchs.

addaleax · 2017-06-07T19:23:58Z

CI: https://ci.nodejs.org/job/node-test-commit/10427/
New AIX CI: https://ci.nodejs.org/job/node-test-commit-aix/6454/

addaleax · 2017-06-08T21:59:08Z

Landed in 02aea06

Re-use the `init` function wherever possible, and move `try { … } catch` blocks that result in fatal errors to a larger scope. Also make the argument order for `init()` consistent in the codebase. PR-URL: nodejs#13419 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Andreas Madsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Re-use the `init` function wherever possible, and move `try { … } catch` blocks that result in fatal errors to a larger scope. Also make the argument order for `init()` consistent in the codebase. PR-URL: #13419 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Andreas Madsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. async_wrap c++ Issues and PRs that require attention from people who are familiar with C++. labels Jun 2, 2017

refack approved these changes Jun 2, 2017

View reviewed changes

AndreasMadsen reviewed Jun 2, 2017

View reviewed changes

refack reviewed Jun 3, 2017

View reviewed changes

addaleax force-pushed the async-hooks-callback-calling-refactor branch from b347faa to 91a6e96 Compare June 3, 2017 09:43

AndreasMadsen approved these changes Jun 4, 2017

View reviewed changes

trevnorris approved these changes Jun 6, 2017

View reviewed changes

async_hooks: minor refactor to callback invocation

868abac

Re-use the `init` function wherever possible, and move `try { … } catch` blocks that result in fatal errors to a larger scope. Also make the argument order for `init()` consistent in the codebase.

addaleax force-pushed the async-hooks-callback-calling-refactor branch from 91a6e96 to 868abac Compare June 7, 2017 19:15

addaleax closed this Jun 8, 2017

addaleax deleted the async-hooks-callback-calling-refactor branch June 8, 2017 21:59

addaleax mentioned this pull request Jun 10, 2017

v8.1.1 proposal #13598

Merged

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

refack mentioned this pull request Jul 9, 2017

async_hooks: simplify fatalError #14051

Closed

3 tasks

MylesBorins added the dont-land-on-v6.x label Jul 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

async_hooks: minor refactor to callback invocation #13419

async_hooks: minor refactor to callback invocation #13419

addaleax commented Jun 2, 2017

refack left a comment

refack Jun 2, 2017

addaleax Jun 2, 2017

refack Jun 2, 2017

trevnorris Jun 6, 2017

refack Jun 6, 2017

trevnorris Jun 6, 2017

AndreasMadsen Jun 2, 2017

addaleax Jun 3, 2017

trevnorris Jun 6, 2017

refack Jun 3, 2017

addaleax Jun 3, 2017

addaleax commented Jun 3, 2017

AndreasMadsen left a comment

trevnorris commented Jun 6, 2017

trevnorris left a comment

addaleax commented Jun 7, 2017

addaleax commented Jun 7, 2017 •

edited

Loading

addaleax commented Jun 8, 2017

		@@ -412,11 +397,17 @@ function emitDestroyS(asyncId) {


		function emitDestroyN(asyncId) {
		if (async_hook_fields[kDestroy] === 0) return;

async_hooks: minor refactor to callback invocation #13419

async_hooks: minor refactor to callback invocation #13419

Conversation

addaleax commented Jun 2, 2017

Checklist

Affected core subsystem(s)

refack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Jun 3, 2017

AndreasMadsen left a comment

Choose a reason for hiding this comment

trevnorris commented Jun 6, 2017

trevnorris left a comment

Choose a reason for hiding this comment

addaleax commented Jun 7, 2017

addaleax commented Jun 7, 2017 • edited Loading

addaleax commented Jun 8, 2017

addaleax commented Jun 7, 2017 •

edited

Loading