domain, src: clean up domain-related code by apapirovski · Pull Request #18291 · nodejs/node

apapirovski · 2018-01-22T16:40:26Z

Move the majority of C++ domain-related code into JS land by introducing a top level domain callback which handles entering & exiting the domain.

Move the rest of the domain necessities into their own file that creates an internal binding, to avoid exposing domain-related code on the process object.

Also a bit of other assorted domain-related cleanup.

(This happens to boost the performance of top-level domain code by roughly 33% but mostly I just wanted to have less domain-related code scattered all over.)

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

domain, src

apapirovski · 2018-01-22T16:41:14Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12657/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1219/

Ugh, I forgot we support older complier versions. Need to get rid of the variable length array.

Move the majority of C++ domain-related code into JS land by introducing a top level domain callback which handles entering & exiting the domain. Move the rest of the domain necessities into their own file that creates an internal binding, to avoid exposing domain-related code on the process object. Modify an existing test slightly to better test domain-related code.

apapirovski · 2018-01-22T17:02:56Z

Fixed up.

CI: https://ci.nodejs.org/job/node-test-pull-request/12658/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1220/

Linter failure is not from this PR...

bnoordhuis

LGTM with a suggestion.

bnoordhuis · 2018-01-22T17:14:51Z

src/node.cc

+      argv_array->Set(env->context(), i, argv[i]).FromJust();
    }
+    Local<Value> domain_argv[] = { callback, argv_array };
+    ret = domain_cb->Call(env->context(), recv, 2, domain_argv);


It's probably faster to use spread in JS land and skip allocating the JS array here. I.e.:

std::vector<Local<Value>> args(1 + argc); args[0] = callback; std::copy(&argv[0], &argv[argc], &args[1]); ret = domain_cb->Call(env->context(), recv, args.size(), &args[0]);

Then in JS:

function topLevelDomainCallback(cb, ...args) { // ... }

Thanks @bnoordhuis, will update!

apapirovski · 2018-01-22T18:28:40Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12660/

Now that linter & inspector tests are fixed.

apapirovski · 2018-01-25T16:44:59Z

@AndreasMadsen I would love to have you review this, if possible, as you've headed up most of the effort around the recent changes to domains. Thanks!

PR-URL: #18291 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Move the majority of C++ domain-related code into JS land by introducing a top level domain callback which handles entering & exiting the domain. Move the rest of the domain necessities into their own file that creates an internal binding, to avoid exposing domain-related code on the process object. Modify an existing test slightly to better test domain-related code. PR-URL: #18291 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

apapirovski · 2018-01-29T16:45:39Z

Landed in 7d3a302, fdf107a, e4743ab and eeede3b

MylesBorins · 2018-02-20T22:48:22Z

this doesn't land cleanly in v9.x, should it be backported?

Move the majority of C++ domain-related code into JS land by introducing a top level domain callback which handles entering & exiting the domain. Move the rest of the domain necessities into their own file that creates an internal binding, to avoid exposing domain-related code on the process object. Modify an existing test slightly to better test domain-related code. PR-URL: #18291 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2018-02-27T07:12:21Z

Does this make sense for LTS?

PR-URL: nodejs#18291 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2018-03-20T15:47:25Z

Should land with #18460

PR-URL: nodejs#18291 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Move the majority of C++ domain-related code into JS land by introducing a top level domain callback which handles entering & exiting the domain. Move the rest of the domain necessities into their own file that creates an internal binding, to avoid exposing domain-related code on the process object. Modify an existing test slightly to better test domain-related code. PR-URL: nodejs#18291 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2018-05-23T17:58:07Z

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

If backporting please include #18460

apapirovski added 3 commits January 21, 2018 13:10

src: remove unnecessary block scope

fab0c98

src: remove unnecessary async hooks check

0d66284

src: remove outdated domain reference

9fdf293

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 22, 2018

apapirovski force-pushed the patch-domain-improvements branch from 3f37b75 to c6cadb3 Compare January 22, 2018 17:02

bnoordhuis approved these changes Jan 22, 2018

View reviewed changes

fixup: bnoordhuis suggestion + test adjustment

930afeb

bnoordhuis approved these changes Jan 22, 2018

View reviewed changes

addaleax approved these changes Jan 22, 2018

View reviewed changes

apapirovski requested a review from AndreasMadsen January 25, 2018 16:44

jasnell approved these changes Jan 26, 2018

View reviewed changes

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

apapirovski added a commit that referenced this pull request Jan 29, 2018

src: remove unnecessary block scope

7d3a302

PR-URL: #18291 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

apapirovski added a commit that referenced this pull request Jan 29, 2018

src: remove unnecessary async hooks check

fdf107a

PR-URL: #18291 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

apapirovski added a commit that referenced this pull request Jan 29, 2018

src: remove outdated domain reference

e4743ab

PR-URL: #18291 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

apapirovski closed this Jan 29, 2018

apapirovski deleted the patch-domain-improvements branch January 29, 2018 16:45

apapirovski removed the request for review from AndreasMadsen January 29, 2018 16:45

AndreasMadsen mentioned this pull request Jan 29, 2018

domain migration to async_hooks #17143

Open

7 tasks

apapirovski mentioned this pull request Jan 30, 2018

src: fix vector subscript out of range #18460

Closed

2 tasks

MylesBorins added the backport-requested-v9.x label Feb 20, 2018

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

MylesBorins added lts-watch-v6.x labels Feb 27, 2018

addaleax removed the backport-requested-v9.x label Feb 27, 2018

addaleax mentioned this pull request Feb 27, 2018

v9.7.0 proposal #19040

Closed

MylesBorins added backport-requested-v8.x and removed lts-watch-v6.x labels May 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

domain, src: clean up domain-related code#18291

domain, src: clean up domain-related code#18291
apapirovski wants to merge 5 commits intonodejs:masterfrom
apapirovski:patch-domain-improvements

apapirovski commented Jan 22, 2018 •

edited

Loading

Uh oh!

apapirovski commented Jan 22, 2018 •

edited

Loading

Uh oh!

apapirovski commented Jan 22, 2018 •

edited

Loading

Uh oh!

bnoordhuis left a comment

Uh oh!

bnoordhuis Jan 22, 2018

Uh oh!

apapirovski Jan 22, 2018

Uh oh!

apapirovski commented Jan 22, 2018

Uh oh!

apapirovski commented Jan 25, 2018

Uh oh!

apapirovski commented Jan 29, 2018

Uh oh!

MylesBorins commented Feb 20, 2018

Uh oh!

MylesBorins commented Feb 27, 2018

Uh oh!

MylesBorins commented Mar 20, 2018

Uh oh!

MylesBorins commented May 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Conversation

apapirovski commented Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

apapirovski commented Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apapirovski commented Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Jan 22, 2018

Choose a reason for hiding this comment

Uh oh!

apapirovski Jan 22, 2018

Choose a reason for hiding this comment

Uh oh!

apapirovski commented Jan 22, 2018

Uh oh!

apapirovski commented Jan 25, 2018

Uh oh!

apapirovski commented Jan 29, 2018

Uh oh!

MylesBorins commented Feb 20, 2018

Uh oh!

MylesBorins commented Feb 27, 2018

Uh oh!

MylesBorins commented Mar 20, 2018

Uh oh!

MylesBorins commented May 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

apapirovski commented Jan 22, 2018 •

edited

Loading

apapirovski commented Jan 22, 2018 •

edited

Loading

apapirovski commented Jan 22, 2018 •

edited

Loading