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

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

Closed
apapirovski wants to merge 5 commits intonodejs:masterfrom
apapirovski:patch-domain-improvements
Closed

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

Conversation

@apapirovski
Copy link
Copy Markdown
Contributor

@apapirovski apapirovski commented Jan 22, 2018 •
edited
Loading

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

@nodejs-github-bot 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
Copy link
Copy Markdown
Contributor Author

apapirovski commented Jan 22, 2018 •
edited
Loading

@apapirovski
domain: further abstract usage in C++
c6cadb3 
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 apapirovski force-pushed the patch-domain-improvements branch from 3f37b75 to c6cadb3 Compare January 22, 2018 17:02
@apapirovski
Copy link
Copy Markdown
Contributor Author

apapirovski commented Jan 22, 2018 •
edited
Loading

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
bnoordhuis approved these changes Jan 22, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM with a suggestion.

Comment thread src/node.cc Outdated
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);
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Jan 22, 2018

Choose a reason for hiding this comment

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

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) {
  // ...
}

Copy link
Copy Markdown
Contributor Author

@apapirovski apapirovski Jan 22, 2018

Choose a reason for hiding this comment

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

Thanks @bnoordhuis, will update!

@apapirovski
Copy link
Copy Markdown
Contributor Author

apapirovski commented Jan 22, 2018

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

Now that linter & inspector tests are fixed.

@apapirovski
Copy link
Copy Markdown
Contributor Author

apapirovski commented Jan 25, 2018

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

apapirovski added a commit that referenced this pull request Jan 29, 2018
@apapirovski
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
@apapirovski
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
@apapirovski
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 added a commit that referenced this pull request Jan 29, 2018
@apapirovski
domain: further abstract usage in C++
eeede3b 
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
Copy link
Copy Markdown
Contributor Author

apapirovski commented Jan 29, 2018

Landed in 7d3a302, fdf107a, e4743ab and eeede3b

@apapirovski apapirovski deleted the patch-domain-improvements branch January 29, 2018 16:45
@apapirovski apapirovski removed the request for review from AndreasMadsen January 29, 2018 16:45
@AndreasMadsen AndreasMadsen mentioned this pull request Jan 29, 2018
Open
7 tasks
@apapirovski apapirovski mentioned this pull request Jan 30, 2018
Closed
2 tasks
@MylesBorins
Copy link
Copy Markdown
Contributor

MylesBorins commented Feb 20, 2018

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

MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
@apapirovski @MylesBorins
domain: further abstract usage in C++
f4fbcf3 
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 pushed a commit that referenced this pull request Feb 21, 2018
@apapirovski @MylesBorins
domain: further abstract usage in C++
4296c63 
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 pushed a commit that referenced this pull request Feb 21, 2018
@apapirovski @MylesBorins
domain: further abstract usage in C++
44bf0f4 
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 MylesBorins mentioned this pull request Feb 21, 2018
Merged
@MylesBorins
Copy link
Copy Markdown
Contributor

MylesBorins commented Feb 27, 2018

Does this make sense for LTS?

addaleax pushed a commit to addaleax/node that referenced this pull request Feb 27, 2018
@apapirovski @addaleax
src: remove unnecessary async hooks check
296523a 
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>
@addaleax addaleax mentioned this pull request Feb 27, 2018
Closed
@MylesBorins
Copy link
Copy Markdown
Contributor

MylesBorins commented Mar 20, 2018

Should land with #18460

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@apapirovski @MayaLekova
src: remove unnecessary block scope
07b16f5 
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>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@apapirovski @MayaLekova
src: remove unnecessary async hooks check
281f4d3 
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>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@apapirovski @MayaLekova
src: remove outdated domain reference
b9e8528 
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>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@apapirovski @MayaLekova
domain: further abstract usage in C++
e48f648 
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
Copy link
Copy Markdown
Contributor

MylesBorins commented May 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. domain Issues and PRs related to the domain subsystem. lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@apapirovski @MylesBorins @bnoordhuis @jasnell @addaleax @AndreasMadsen @nodejs-github-bot