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 more version computation code into node_metadata.cc #25115

Closed
wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

src: move the declaration of http parser versions into node_metadata.h

Instead of putting them in node_internals.h.

src: move GetOpenSSLVersion into node_metadata.cc

Instead of implementing it in node_crypto.cc even though the only
place that needs it is the Metadata::Versions constructor.

src: initialize ICU version in per_process::metadata.versions

Instead of

  • Initialize the ICU versions in JS land after consulting
    internalBinding('config').hasIntl
  • Joining the version keys in C++
  • Splitting the keys in JS and call into C++ again to get the value for
    each of the keys

Do:

  • Guard the initialization code behind NODE_HAVE_I18N_SUPPORT
  • Do the initialization in C++ right after ICU data is loaded
  • Initialize each version directly using ICU functions/constants,
    and put them in per_process::metadata.versions. These will be
    copied into process.versions naturally later.
    This way, the initialization of the versions won't be called
    in worker threads again.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Instead of implementing it in node_crypto.cc even though the only
place that needs it is the `Metadata::Versions` constructor.
Instead of

- Initialize the ICU versions in JS land after consulting
  internalBinding('config').hasIntl
- Joining the version keys in C++
- Splitting the keys in JS and call into C++ again to get the value for
  each of the keys

Do:

- Guard the initialization code behind `NODE_HAVE_I18N_SUPPORT`
- Do the initialization in C++ right after ICU data is loaded
- Initialize each version directly using ICU functions/constants,
  and put them in per_process::metadata.versions. These will be
  copied into `process.versions` naturally later.
  This way, the initialization of the versions won't be called
  in worker threads again.
@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 Dec 18, 2018
src/node_i18n.cc Show resolved Hide resolved
@joyeecheung
Copy link
Member Author

@Trott
Copy link
Member

Trott commented Dec 19, 2018

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19667/ ✔️

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 19, 2018
@joyeecheung
Copy link
Member Author

Landed in 3438f4b...263d137, thanks!

pull bot pushed a commit to SimenB/node that referenced this pull request Dec 20, 2018
Instead of implementing it in node_crypto.cc even though the only
place that needs it is the `Metadata::Versions` constructor.

PR-URL: nodejs#25115
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
pull bot pushed a commit to SimenB/node that referenced this pull request Dec 20, 2018
Instead of putting them in node_internals.h.

PR-URL: nodejs#25115
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
pull bot pushed a commit to SimenB/node that referenced this pull request Dec 20, 2018
Instead of

- Initialize the ICU versions in JS land after consulting
  internalBinding('config').hasIntl
- Joining the version keys in C++
- Splitting the keys in JS and call into C++ again to get the value for
  each of the keys

Do:

- Guard the initialization code behind `NODE_HAVE_I18N_SUPPORT`
- Do the initialization in C++ right after ICU data is loaded
- Initialize each version directly using ICU functions/constants,
  and put them in per_process::metadata.versions. These will be
  copied into `process.versions` naturally later.
  This way, the initialization of the versions won't be called
  in worker threads again.

PR-URL: nodejs#25115
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

This doesn't land cleanly on v11.x, should it be backported?

@addaleax
Copy link
Member

Applies cleanly now.

addaleax pushed a commit that referenced this pull request Jan 14, 2019
Instead of implementing it in node_crypto.cc even though the only
place that needs it is the `Metadata::Versions` constructor.

PR-URL: #25115
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Jan 14, 2019
Instead of putting them in node_internals.h.

PR-URL: #25115
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Jan 14, 2019
Instead of

- Initialize the ICU versions in JS land after consulting
  internalBinding('config').hasIntl
- Joining the version keys in C++
- Splitting the keys in JS and call into C++ again to get the value for
  each of the keys

Do:

- Guard the initialization code behind `NODE_HAVE_I18N_SUPPORT`
- Do the initialization in C++ right after ICU data is loaded
- Initialize each version directly using ICU functions/constants,
  and put them in per_process::metadata.versions. These will be
  copied into `process.versions` naturally later.
  This way, the initialization of the versions won't be called
  in worker threads again.

PR-URL: #25115
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Instead of implementing it in node_crypto.cc even though the only
place that needs it is the `Metadata::Versions` constructor.

PR-URL: nodejs#25115
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Instead of putting them in node_internals.h.

PR-URL: nodejs#25115
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Instead of

- Initialize the ICU versions in JS land after consulting
  internalBinding('config').hasIntl
- Joining the version keys in C++
- Splitting the keys in JS and call into C++ again to get the value for
  each of the keys

Do:

- Guard the initialization code behind `NODE_HAVE_I18N_SUPPORT`
- Do the initialization in C++ right after ICU data is loaded
- Initialize each version directly using ICU functions/constants,
  and put them in per_process::metadata.versions. These will be
  copied into `process.versions` naturally later.
  This way, the initialization of the versions won't be called
  in worker threads again.

PR-URL: nodejs#25115
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
Instead of implementing it in node_crypto.cc even though the only
place that needs it is the `Metadata::Versions` constructor.

PR-URL: nodejs#25115
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
Instead of putting them in node_internals.h.

PR-URL: nodejs#25115
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
Instead of

- Initialize the ICU versions in JS land after consulting
  internalBinding('config').hasIntl
- Joining the version keys in C++
- Splitting the keys in JS and call into C++ again to get the value for
  each of the keys

Do:

- Guard the initialization code behind `NODE_HAVE_I18N_SUPPORT`
- Do the initialization in C++ right after ICU data is loaded
- Initialize each version directly using ICU functions/constants,
  and put them in per_process::metadata.versions. These will be
  copied into `process.versions` naturally later.
  This way, the initialization of the versions won't be called
  in worker threads again.

PR-URL: nodejs#25115
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants