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.

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

src: move per-process global variables into node::per_process #25302

Closed
wants to merge 2 commits into from

Conversation

@joyeecheung
Copy link
Member

commented Jan 1, 2019

So that it's easier to tell whether we are manipulating per-process
global states that may need to be treated with care to avoid races.

Also added comments about these variables and moved some of them
to a more suitable compilation unit:

  • Move v8_initialized to util.h since it's only used in
    util.cc and node.cc
  • Rename process_mutex to tty_mutex and move it into
    node_errors.cc since that's the only place it's used
    to guard the tty.
  • Move per_process_opts_mutex and per_process_opts
    into node_options.h and rename them to
    per_process::cli_options[_mutex]
  • Rename node_isolate[_mutex] to per_process::main_isolate[_mutex]
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@joyeecheung joyeecheung requested a review from addaleax Jan 1, 2019

unsigned int reverted = 0;
// Isolate on the main thread
static Mutex main_isolate_mutex;
static Isolate* main_isolate;

This comment has been minimized.

Copy link
@addaleax

addaleax Jan 1, 2019

Member

Are these actually consumed anywhere? It looks like we should remove this entirely…

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Jan 1, 2019

Author Member

I think they are used to check that someone don't call Start multiple times and accidentally dispose the isolate being used in another thread..though I am having a hard time imaging how that would be possible with the current code...

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Jan 3, 2019

Member

node_isolate is kind of vestigial at this point, see commit 75adde0 from 2014. We couldn't remove it outright back then but we can now.

The mutex was introduced to fix a SIGUSR1 race, see commit 844f0a9. It too can be removed.

@jasnell
jasnell approved these changes Jan 1, 2019
@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jan 2, 2019

src/node.cc Outdated
@@ -139,21 +139,28 @@ using v8::Undefined;
using v8::V8;
using v8::Value;

namespace per_process {
// Tells whether --prof is passed.
// TODO(joyeecheung): replace this with env->options()->prof_process

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Jan 3, 2019

Member

I don't know if that would do any good. --prof is a global flag.

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Jan 4, 2019

Author Member

@bnoordhuis Good catch...we should probably move that to per_process::cli_options.prof_process

unsigned int reverted = 0;
// Isolate on the main thread
static Mutex main_isolate_mutex;
static Isolate* main_isolate;

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Jan 3, 2019

Member

node_isolate is kind of vestigial at this point, see commit 75adde0 from 2014. We couldn't remove it outright back then but we can now.

The mutex was introduced to fix a SIGUSR1 race, see commit 844f0a9. It too can be removed.

joyeecheung added 2 commits Jan 1, 2019
src: move per-process global variables into node::per_process
So that it's easier to tell whether we are manipulating per-process
global states that may need to be treated with care to avoid races.

Also added comments about these variables and moved some of them
to a more suitable compilation unit:

- Move `v8_initialized` to `util.h` since it's only used in
  `util.cc` and `node.cc`
- Rename `process_mutex` to `tty_mutex` and move it into
  `node_errors.cc` since that's the only place it's used
  to guard the tty.
- Move `per_process_opts_mutex` and `per_process_opts`
  into `node_options.h` and rename them to
  `per_process::cli_options[_mutex]`
- Rename `node_isolate[_mutex]` to `per_process::main_isolate[_mutex]`

@joyeecheung joyeecheung force-pushed the joyeecheung:per-process branch from c8829cf to 3a58828 Jan 4, 2019

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jan 4, 2019

@bnoordhuis Thanks for the reviews, I've modified the TODOs. This is more of a stylish change, so I will address those in subsequence PRs...just in case there is anything we overlook.

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jan 4, 2019

@fhinkel
fhinkel approved these changes Jan 5, 2019
@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jan 6, 2019

@bnoordhuis
Copy link
Member

left a comment

LGTM % style nit. Nice work, Joyee, I think this makes the code much more readable.

Aside: was the overhead of using std::shared_ptr discussed?

node::UncheckedMalloc(length);
return per_process::cli_options->zero_fill_all_buffers
? node::UncheckedCalloc(length)
: node::UncheckedMalloc(length);

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Jan 6, 2019

Member

? / : on a new line is not a style we use anywhere else, I think. It always goes on the same line as the clause preceding it.

@addaleax

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

Landed in 9db9e7e (with @bnoordhuis’s nit addressed) 🙂

@addaleax addaleax closed this Jan 7, 2019

addaleax added a commit that referenced this pull request Jan 7, 2019
src: move per-process global variables into node::per_process
So that it's easier to tell whether we are manipulating per-process
global states that may need to be treated with care to avoid races.

Also added comments about these variables and moved some of them
to a more suitable compilation unit:

- Move `v8_initialized` to `util.h` since it's only used in
  `util.cc` and `node.cc`
- Rename `process_mutex` to `tty_mutex` and move it into
  `node_errors.cc` since that's the only place it's used
  to guard the tty.
- Move `per_process_opts_mutex` and `per_process_opts`
  into `node_options.h` and rename them to
  `per_process::cli_options[_mutex]`
- Rename `node_isolate[_mutex]` to `per_process::main_isolate[_mutex]`

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

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

This needs to be backported to v11.x.

@BridgeAR BridgeAR added this to Backport requested in v11.x Jan 10, 2019

refack added a commit to refack/node that referenced this pull request Jan 14, 2019
src: move per-process global variables into node::per_process
So that it's easier to tell whether we are manipulating per-process
global states that may need to be treated with care to avoid races.

Also added comments about these variables and moved some of them
to a more suitable compilation unit:

- Move `v8_initialized` to `util.h` since it's only used in
  `util.cc` and `node.cc`
- Rename `process_mutex` to `tty_mutex` and move it into
  `node_errors.cc` since that's the only place it's used
  to guard the tty.
- Move `per_process_opts_mutex` and `per_process_opts`
  into `node_options.h` and rename them to
  `per_process::cli_options[_mutex]`
- Rename `node_isolate[_mutex]` to `per_process::main_isolate[_mutex]`

PR-URL: nodejs#25302
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
addaleax added a commit that referenced this pull request Jan 15, 2019
src: move per-process global variables into node::per_process
So that it's easier to tell whether we are manipulating per-process
global states that may need to be treated with care to avoid races.

Also added comments about these variables and moved some of them
to a more suitable compilation unit:

- Move `v8_initialized` to `util.h` since it's only used in
  `util.cc` and `node.cc`
- Rename `process_mutex` to `tty_mutex` and move it into
  `node_errors.cc` since that's the only place it's used
  to guard the tty.
- Move `per_process_opts_mutex` and `per_process_opts`
  into `node_options.h` and rename them to
  `per_process::cli_options[_mutex]`
- Rename `node_isolate[_mutex]` to `per_process::main_isolate[_mutex]`

PR-URL: #25302
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@BridgeAR BridgeAR referenced this pull request Jan 16, 2019
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
src: move per-process global variables into node::per_process
So that it's easier to tell whether we are manipulating per-process
global states that may need to be treated with care to avoid races.

Also added comments about these variables and moved some of them
to a more suitable compilation unit:

- Move `v8_initialized` to `util.h` since it's only used in
  `util.cc` and `node.cc`
- Rename `process_mutex` to `tty_mutex` and move it into
  `node_errors.cc` since that's the only place it's used
  to guard the tty.
- Move `per_process_opts_mutex` and `per_process_opts`
  into `node_options.h` and rename them to
  `per_process::cli_options[_mutex]`
- Rename `node_isolate[_mutex]` to `per_process::main_isolate[_mutex]`

PR-URL: nodejs#25302
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
src: move per-process global variables into node::per_process
So that it's easier to tell whether we are manipulating per-process
global states that may need to be treated with care to avoid races.

Also added comments about these variables and moved some of them
to a more suitable compilation unit:

- Move `v8_initialized` to `util.h` since it's only used in
  `util.cc` and `node.cc`
- Rename `process_mutex` to `tty_mutex` and move it into
  `node_errors.cc` since that's the only place it's used
  to guard the tty.
- Move `per_process_opts_mutex` and `per_process_opts`
  into `node_options.h` and rename them to
  `per_process::cli_options[_mutex]`
- Rename `node_isolate[_mutex]` to `per_process::main_isolate[_mutex]`

PR-URL: nodejs#25302
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins referenced this pull request Jan 24, 2019

@targos targos removed this from Backport requested in v11.x Jan 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.