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

+ 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:
functiontopLevelDomainCallback(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 c6cadb3Compare 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 993b716Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952Compare 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
Closed
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

Navigation Menu

Uh oh!

Uh oh!

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

domain, src: clean up domain-related code#18291
apapirovski wants to merge 5 commits into
nodejs: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!

bnoordhuisJan 22, 2018

Uh oh!

apapirovskiJan 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!

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!

bnoordhuisJan 22, 2018

Choose a reason for hiding this comment

Uh oh!

apapirovskiJan 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