[v11.x] src: move more process methods initialization in bootstrap/node.js - #25496
Closed
addaleax wants to merge 8 commits into
Closed
[v11.x] src: move more process methods initialization in bootstrap/node.js#25496addaleax wants to merge 8 commits into
addaleax wants to merge 8 commits into
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
Instead of:
- Writing methods onto the process directly in C++ during
`SetupProcessObject()` and overwrite with argument checks later
- Or, wrapping and writing them in `internal/process/*.js`
Do:
- Move the C++ implementations in node_process.cc and mark them static
wherever possible
- Expose the C++ methods through a new
`internalBinding('process_methods')`
- Wrap the methods in `internal/process/*.js` in a
side-effect-free manner and return them back to
`internal/bootstrap/node.js`
- Centralize the write to the process object based on conditions
in `bootstrap/node.js`
So it's easier to see what methods are attached to the process object
during bootstrap under what condition and in what order.
The eventual goal is to figure out the dependency of process methods
and the write/read access to the process object during bootstrap, group
these access properly and remove the process properties that should not
be exposed to users this way.
Also correct the NODE_PERFORMANCE_MILESTONE_BOOTSTRAP_COMPLETE milestone
which should be marked before code execution.
Refs: nodejs#24961
PR-URL: nodejs#25127
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
The warnings in question are:
../src/node.cc:844:13: warning:
unused function 'DebugProcess' [-Wunused-function]
static void DebugProcess(const FunctionCallbackInfo<Value>& args);
^
../src/node.cc:845:13: warning:
unused function 'DebugEnd' [-Wunused-function]
static void DebugEnd(const FunctionCallbackInfo<Value>& args);
PR-URL: nodejs#25165
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Copy link
Copy Markdown
Collaborator
|
@addaleax sadly an error occured when I tried to trigger a build :( |
Sorry, something went wrong.
This comment has been minimized.
console is not ready to use at this point in the bootstrapping process, so switch to process._rawDebug() instead. PR-URL: nodejs#25289 Fixes: nodejs#25287 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
During coverage setup, path.resolve() is called. path.resolve() can potentially call process.cwd(), which hasn't been bootstrapped yet. This commit passes the current working directory directly so that path.resolve() doesn't attempt to compute it. PR-URL: nodejs#25289 Fixes: nodejs#25287 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Update the coverage test to verify that nothing is printed to stderr (which happens when coverage errors happen). Also add a test case to verify that non-absolute coverage paths work. PR-URL: nodejs#25289 Fixes: nodejs#25287 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax
force-pushed
the
backport-v11.x-25127
branch
from
21f49e4 to
066985a
Compare
January 14, 2019 16:23
Copy link
Copy Markdown
Member
Author
|
CI: https://ci.nodejs.org/job/node-test-pull-request/20108/ (:white_check_mark:) |
Sorry, something went wrong.
PR-URL: nodejs#25103 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Move `node::errno_string` into node_errors.h/cc and move it into the `node:errors` namespace to reduce the size of the header. It's not on any performance-critical code path so does not need to be inlined. PR-URL: nodejs#25396 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: nodejs#25439 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Masashi Hirano <shisama07@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung
approved these changes
Jan 15, 2019
addaleax
pushed a commit
that referenced
this pull request
Jan 15, 2019
Instead of:
- Writing methods onto the process directly in C++ during
`SetupProcessObject()` and overwrite with argument checks later
- Or, wrapping and writing them in `internal/process/*.js`
Do:
- Move the C++ implementations in node_process.cc and mark them static
wherever possible
- Expose the C++ methods through a new
`internalBinding('process_methods')`
- Wrap the methods in `internal/process/*.js` in a
side-effect-free manner and return them back to
`internal/bootstrap/node.js`
- Centralize the write to the process object based on conditions
in `bootstrap/node.js`
So it's easier to see what methods are attached to the process object
during bootstrap under what condition and in what order.
The eventual goal is to figure out the dependency of process methods
and the write/read access to the process object during bootstrap, group
these access properly and remove the process properties that should not
be exposed to users this way.
Also correct the NODE_PERFORMANCE_MILESTONE_BOOTSTRAP_COMPLETE milestone
which should be marked before code execution.
Refs: #24961
PR-URL: #25127
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Backport-PR-URL: #25496
addaleax
pushed a commit
that referenced
this pull request
Jan 15, 2019
The warnings in question are:
../src/node.cc:844:13: warning:
unused function 'DebugProcess' [-Wunused-function]
static void DebugProcess(const FunctionCallbackInfo<Value>& args);
^
../src/node.cc:845:13: warning:
unused function 'DebugEnd' [-Wunused-function]
static void DebugEnd(const FunctionCallbackInfo<Value>& args);
PR-URL: #25165
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Backport-PR-URL: #25496
addaleax
pushed a commit
that referenced
this pull request
Jan 15, 2019
console is not ready to use at this point in the bootstrapping process, so switch to process._rawDebug() instead. PR-URL: #25289 Fixes: #25287 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Backport-PR-URL: #25496
addaleax
pushed a commit
that referenced
this pull request
Jan 15, 2019
During coverage setup, path.resolve() is called. path.resolve() can potentially call process.cwd(), which hasn't been bootstrapped yet. This commit passes the current working directory directly so that path.resolve() doesn't attempt to compute it. PR-URL: #25289 Fixes: #25287 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Backport-PR-URL: #25496
addaleax
pushed a commit
that referenced
this pull request
Jan 15, 2019
Update the coverage test to verify that nothing is printed to stderr (which happens when coverage errors happen). Also add a test case to verify that non-absolute coverage paths work. PR-URL: #25289 Fixes: #25287 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Backport-PR-URL: #25496
BridgeAR
pushed a commit
to BridgeAR/node
that referenced
this pull request
Jan 16, 2019
Instead of:
- Writing methods onto the process directly in C++ during
`SetupProcessObject()` and overwrite with argument checks later
- Or, wrapping and writing them in `internal/process/*.js`
Do:
- Move the C++ implementations in node_process.cc and mark them static
wherever possible
- Expose the C++ methods through a new
`internalBinding('process_methods')`
- Wrap the methods in `internal/process/*.js` in a
side-effect-free manner and return them back to
`internal/bootstrap/node.js`
- Centralize the write to the process object based on conditions
in `bootstrap/node.js`
So it's easier to see what methods are attached to the process object
during bootstrap under what condition and in what order.
The eventual goal is to figure out the dependency of process methods
and the write/read access to the process object during bootstrap, group
these access properly and remove the process properties that should not
be exposed to users this way.
Also correct the NODE_PERFORMANCE_MILESTONE_BOOTSTRAP_COMPLETE milestone
which should be marked before code execution.
Refs: nodejs#24961
PR-URL: nodejs#25127
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Backport-PR-URL: nodejs#25496
BridgeAR
pushed a commit
to BridgeAR/node
that referenced
this pull request
Jan 16, 2019
The warnings in question are:
../src/node.cc:844:13: warning:
unused function 'DebugProcess' [-Wunused-function]
static void DebugProcess(const FunctionCallbackInfo<Value>& args);
^
../src/node.cc:845:13: warning:
unused function 'DebugEnd' [-Wunused-function]
static void DebugEnd(const FunctionCallbackInfo<Value>& args);
PR-URL: nodejs#25165
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Backport-PR-URL: nodejs#25496
BridgeAR
pushed a commit
to BridgeAR/node
that referenced
this pull request
Jan 16, 2019
console is not ready to use at this point in the bootstrapping process, so switch to process._rawDebug() instead. PR-URL: nodejs#25289 Fixes: nodejs#25287 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Backport-PR-URL: nodejs#25496
BridgeAR
pushed a commit
to BridgeAR/node
that referenced
this pull request
Jan 16, 2019
During coverage setup, path.resolve() is called. path.resolve() can potentially call process.cwd(), which hasn't been bootstrapped yet. This commit passes the current working directory directly so that path.resolve() doesn't attempt to compute it. PR-URL: nodejs#25289 Fixes: nodejs#25287 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Backport-PR-URL: nodejs#25496
BridgeAR
pushed a commit
to BridgeAR/node
that referenced
this pull request
Jan 16, 2019
Update the coverage test to verify that nothing is printed to stderr (which happens when coverage errors happen). Also add a test case to verify that non-absolute coverage paths work. PR-URL: nodejs#25289 Fixes: nodejs#25287 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Backport-PR-URL: nodejs#25496
This was referenced Aug 9, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Backport #25127, and clean cherry-picks of #25165 and #25289 which should go with it.