GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — src: move per-process global variables into node::per_process by joyeecheung · Pull Request #25302 · nodejs/node

Conversation

joyeecheung commented Jan 1, 2019

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji

All reactions

👍 1 reaction

joyeecheung requested a review from addaleax

January 1, 2019 06:19

nodejs-github-bot commented Jan 1, 2019

Copy link

Copy Markdown

Collaborator

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2120/pipeline

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot added c++

Issues and PRs that require attention from people who are familiar with C++.

lib / src Issues and PRs involving general changes in the lib/ or src/ directories. labels

Jan 1, 2019

addaleax approved these changes

Jan 1, 2019

View reviewed changes

Comment thread

src/node.cc Outdated

addaleax Jan 1, 2019

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

joyeecheung Jan 1, 2019

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bnoordhuis Jan 3, 2019

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

jasnell approved these changes

Jan 1, 2019

View reviewed changes

joyeecheung commented Jan 2, 2019

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnoordhuis reviewed

Jan 3, 2019

View reviewed changes

Comment thread

src/node.cc Outdated

bnoordhuis Jan 3, 2019

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

joyeecheung Jan 4, 2019

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/node.cc Outdated

bnoordhuis Jan 3, 2019

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

joyeecheung added 2 commits

January 5, 2019 00:35


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

11ed69e

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]`


          fixup! src: move per-process global variables into node::per_process

3a58828

joyeecheung force-pushed the per-process branch from c8829cf to 3a58828 Compare

January 4, 2019 16:40

joyeecheung commented Jan 4, 2019

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

joyeecheung commented Jan 4, 2019

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

joyeecheung added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label

Jan 4, 2019

fhinkel approved these changes

Jan 5, 2019

View reviewed changes

joyeecheung commented Jan 6, 2019

Copy link

Copy Markdown

Member Author

Resume: https://ci.nodejs.org/job/node-test-pull-request/19954/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnoordhuis approved these changes

Jan 6, 2019

View reviewed changes

bnoordhuis left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/node_buffer.cc

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

bnoordhuis Jan 6, 2019 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

? / : 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

addaleax commented Jan 7, 2019

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax closed this

Jan 7, 2019

addaleax pushed a commit that referenced this pull request

Jan 7, 2019


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

9db9e7e

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>

addaleax commented Jan 9, 2019

Copy link

Copy Markdown

Member

This needs to be backported to v11.x.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax added the backport-requested-v11.x label

Jan 9, 2019

refack pushed a commit to refack/node that referenced this pull request

Jan 14, 2019


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

a079f68

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>

addaleax removed the backport-requested-v11.x label

Jan 15, 2019

addaleax pushed a commit that referenced this pull request

Jan 15, 2019


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

a8daccc

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>

BridgeAR mentioned this pull request

Jan 16, 2019

v11.7.0 proposal #25537

Merged

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request

Jan 16, 2019


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

253ef35

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request

Jan 17, 2019


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

c6adf4b

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>

MylesBorins mentioned this pull request

Jan 24, 2019

v11.8.0 proposal #25687

Merged

Reviewers

bnoordhuis

bnoordhuis approved these changes

jasnell

jasnell approved these changes

addaleax

addaleax approved these changes

+1 more reviewer

fhinkel

fhinkel approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

c++

Issues and PRs that require attention from people who are familiar with C++.

lib / src

Issues and PRs involving general changes in the lib/ or src/ directories.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

joyeecheung commented Jan 1, 2019

Checklist

Uh oh!

nodejs-github-bot commented Jan 1, 2019

Uh oh!

addaleax Jan 1, 2019

Choose a reason for hiding this comment

Uh oh!

joyeecheung Jan 1, 2019

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Jan 3, 2019

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Jan 2, 2019

Uh oh!

bnoordhuis Jan 3, 2019

Choose a reason for hiding this comment

Uh oh!

joyeecheung Jan 4, 2019

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Jan 3, 2019

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Jan 4, 2019

Uh oh!

joyeecheung commented Jan 4, 2019

Uh oh!

joyeecheung commented Jan 6, 2019

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Jan 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jan 7, 2019

Uh oh!

addaleax commented Jan 9, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bnoordhuis Jan 6, 2019 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.