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.

Sign up for GitHub

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

Jump to bottom

src: move more version computation code into node_metadata.cc #25115

Closed
wants to merge 3 commits into from
Closed

src: move more version computation code into node_metadata.cc #25115

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

@joyeecheung
src: move GetOpenSSLVersion into node_metadata.cc
15a80fc 
Instead of implementing it in node_crypto.cc even though the only
place that needs it is the `Metadata::Versions` constructor.
@joyeecheung
src: move the declaration of http parser versions into node_metadata.h
c6b5f2a 
Instead of putting them in node_internals.h.
@joyeecheung
src: initialize ICU version in per_process::metadata.versions
b864793 
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
Copy link
Collaborator

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

@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
srl295
srl295 approved these changes Dec 18, 2018
View reviewed changes
src/node_i18n.cc Show resolved Hide resolved
@joyeecheung
Copy link
Member Author

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

@Trott
Copy link
Member

Trott commented Dec 19, 2018 •
edited
Loading

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
@joyeecheung
src: move GetOpenSSLVersion into node_metadata.cc
e970361 
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 <srloomis@us.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
pull bot pushed a commit to SimenB/node that referenced this pull request Dec 20, 2018
@joyeecheung
src: move the declaration of http parser versions into node_metadata.h
a29c93a 
Instead of putting them in node_internals.h.

PR-URL: nodejs#25115
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
pull bot pushed a commit to SimenB/node that referenced this pull request Dec 20, 2018
@joyeecheung
src: initialize ICU version in per_process::metadata.versions
263d137 
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 <srloomis@us.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@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
@joyeecheung @addaleax
src: move GetOpenSSLVersion into node_metadata.cc
64c713a 
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 <srloomis@us.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Jan 14, 2019
@joyeecheung @addaleax
src: move the declaration of http parser versions into node_metadata.h
e5b4af4 
Instead of putting them in node_internals.h.

PR-URL: #25115
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Jan 14, 2019
@joyeecheung @addaleax
src: initialize ICU version in per_process::metadata.versions
de6f1f5 
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 <srloomis@us.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@joyeecheung @refack
src: move GetOpenSSLVersion into node_metadata.cc
5b98e53 
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 <srloomis@us.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@joyeecheung @refack
src: move the declaration of http parser versions into node_metadata.h
d522b1e 
Instead of putting them in node_internals.h.

PR-URL: nodejs#25115
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@joyeecheung @refack
src: initialize ICU version in per_process::metadata.versions
df9f162 
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 <srloomis@us.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
Merged
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@joyeecheung @BridgeAR
src: move GetOpenSSLVersion into node_metadata.cc
7926159 
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 <srloomis@us.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@joyeecheung @BridgeAR
src: move the declaration of http parser versions into node_metadata.h
c84f019 
Instead of putting them in node_internals.h.

PR-URL: nodejs#25115
Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@joyeecheung @BridgeAR
src: initialize ICU version in per_process::metadata.versions
930de5b 
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 <srloomis@us.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@JungMinu JungMinu JungMinu approved these changes

@srl295 srl295 srl295 approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax Awaiting requested review from addaleax

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@joyeecheung @nodejs-github-bot @Trott @MylesBorins @addaleax @jasnell @srl295 @JungMinu @richardlau @BethGriggs