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

process: split bootstrappers by threads that can run them #21378

Closed

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jun 17, 2018

The first commit is from #21377

This patch split part of the bootstrappers into three files:

  • lib/internal/process/main_thread_only.js: contains bootstrappers
    that can only be run in the main thread, including
    • setupStdio for the main thread that sets up process.stdin,
      process.stdout, process.error that may interact with external
      resources, e.g. TTY/File/Pipe/TCP sockets
    • setupProcessMethods that setup methods changing process-global
      states, e.g. process.chdir, process.umask, process.setuid
    • setupSignalHandlers
    • setupChildProcessIpcChannel that setup process.send for
      child processes.
  • lib/internal/process/worker_thread_only.js: contains bootstrappers
    that can only be run in the worker threads, including
    • setupStdio for the worker thread that are streams to be
      manipulated or piped to the parent thread
  • lib/internal/process/per_thread.js: contains bootstrappers
    that can be run in all threads, including:
    • setupAssert for process.assert
    • setupCpuUsage for process.cpuUsage
    • setupHrtime for process.hrtime and process.hrtime.bigint
    • setupMemoryUsage for process.memoryUsage
    • setupConfig for process.config
    • setupKillAndExit for process.kill and process.exit
    • setupRawDebug for process._rawDebug
    • setupUncaughtExceptionCapture for
      process.setUncaughtExceptionCaptureCallback and
      process.hasUncaughtExceptionCaptureCallback

Hopefully in the future we can sort more bootstrappers in
boostrap/node.js into these three files and further group
them into functions that can be run before creating the
snapshot / after loading the snapshot.

This patch also moves most of the isMainThread conditionals
into the main bootstrapper instead of letting them scattered around
special-casing different implementations.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jun 17, 2018
@joyeecheung joyeecheung added process Issues and PRs related to the process subsystem. worker Issues and PRs related to Worker support. labels Jun 17, 2018
@joyeecheung
Copy link
Member Author

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

Ping @addaleax if you want to take a look, but it should be pretty safe to land since it's just internal refactoring

This patch split part of the bootstrappers into three files:

- `lib/internal/process/main_thread_only.js`: contains bootstrappers
  that can only be run in the main thread, including
  - `setupStdio` for the main thread that sets up `process.stdin`,
    `process.stdout`, `process.error` that may interact with external
    resources, e.g. TTY/File/Pipe/TCP sockets
  - `setupProcessMethods` that setup methods changing process-global
    states, e.g. `process.chdir`, `process.umask`, `process.setuid`
  - `setupSignalHandlers`
  - `setupChildProcessIpcChannel` that setup `process.send` for
    child processes.
- `lib/internal/process/worker_thread_only.js`: contains bootstrappers
  that can only be run in the worker threads, including
  - `setupStdio` for the worker thread that are streams to be
    manipulated or piped to the parent thread
- `lib/internal/process/per_thread.js`: contains bootstrappers
    that can be run in all threads, including:
  - `setupAssert` for `process.assert`
  - `setupCpuUsage` for `process.cpuUsage`
  - `setupHrtime` for `process.hrtime` and `process.hrtime.bigint`
  - `setupMemoryUsage` for `process.memoryUsage`
  - `setupConfig` for `process.config`
  - `setupKillAndExit` for `process.kill` and `process.exit`
  - `setupRawDebug` for `process._rawDebug`
  - `setupUncaughtExceptionCapture` for
    `process.setUncaughtExceptionCaptureCallback` and
    `process.hasUncaughtExceptionCaptureCallback`

Hopefully in the future we can sort more bootstrappers in
`boostrap/node.js` into these three files and further group
them into functions that can be run before creating the
snapshot / after loading the snapshot.

This patch also moves most of the `isMainThread` conditionals
into the main bootstrapper instead of letting them scattered around
special-casing different implementations.
@joyeecheung
Copy link
Member Author

joyeecheung commented Jun 20, 2018

Rebase since #21377 landed, GitHub said there were conflicts although I applied the commit without any conflicts locally so the patch is still the same (since this PR used to contain a commit from #21377 and after rebase there's only the second commit left).

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

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Looks good and CI is green :)

:shipit:

Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

lgtm

@joyeecheung
Copy link
Member Author

Landed in e43d91c, thanks!

joyeecheung added a commit that referenced this pull request Jun 20, 2018
This patch split part of the bootstrappers into three files:

- `lib/internal/process/main_thread_only.js`: contains bootstrappers
  that can only be run in the main thread, including
  - `setupStdio` for the main thread that sets up `process.stdin`,
    `process.stdout`, `process.error` that may interact with external
    resources, e.g. TTY/File/Pipe/TCP sockets
  - `setupProcessMethods` that setup methods changing process-global
    states, e.g. `process.chdir`, `process.umask`, `process.setuid`
  - `setupSignalHandlers`
  - `setupChildProcessIpcChannel` that setup `process.send` for
    child processes.
- `lib/internal/process/worker_thread_only.js`: contains bootstrappers
  that can only be run in the worker threads, including
  - `setupStdio` for the worker thread that are streams to be
    manipulated or piped to the parent thread
- `lib/internal/process/per_thread.js`: contains bootstrappers
    that can be run in all threads, including:
  - `setupAssert` for `process.assert`
  - `setupCpuUsage` for `process.cpuUsage`
  - `setupHrtime` for `process.hrtime` and `process.hrtime.bigint`
  - `setupMemoryUsage` for `process.memoryUsage`
  - `setupConfig` for `process.config`
  - `setupKillAndExit` for `process.kill` and `process.exit`
  - `setupRawDebug` for `process._rawDebug`
  - `setupUncaughtExceptionCapture` for
    `process.setUncaughtExceptionCaptureCallback` and
    `process.hasUncaughtExceptionCaptureCallback`

Hopefully in the future we can sort more bootstrappers in
`boostrap/node.js` into these three files and further group
them into functions that can be run before creating the
snapshot / after loading the snapshot.

This patch also moves most of the `isMainThread` conditionals
into the main bootstrapper instead of letting them scattered around
special-casing different implementations.

PR-URL: #21378
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@targos targos added backport-requested-v10.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jun 20, 2018
@targos
Copy link
Member

targos commented Jun 20, 2018

Should land on v10.x-staging after #21378 #21256

@joyeecheung
Copy link
Member Author

@targos Did you mean #21377?

@targos
Copy link
Member

targos commented Jun 20, 2018

@joyeecheung no, sorry. I meant #21256

@targos
Copy link
Member

targos commented Jul 14, 2018

@joyeecheung Previous PRs now landed on v10.x-staging. Would you be willing to backport this one now?

@mcollina mcollina mentioned this pull request Jul 17, 2018
@targos
Copy link
Member

targos commented Jul 18, 2018

Ping

joyeecheung added a commit to joyeecheung/node that referenced this pull request Jul 18, 2018
This patch split part of the bootstrappers into three files:

- `lib/internal/process/main_thread_only.js`: contains bootstrappers
  that can only be run in the main thread, including
  - `setupStdio` for the main thread that sets up `process.stdin`,
    `process.stdout`, `process.error` that may interact with external
    resources, e.g. TTY/File/Pipe/TCP sockets
  - `setupProcessMethods` that setup methods changing process-global
    states, e.g. `process.chdir`, `process.umask`, `process.setuid`
  - `setupSignalHandlers`
  - `setupChildProcessIpcChannel` that setup `process.send` for
    child processes.
- `lib/internal/process/worker_thread_only.js`: contains bootstrappers
  that can only be run in the worker threads, including
  - `setupStdio` for the worker thread that are streams to be
    manipulated or piped to the parent thread
- `lib/internal/process/per_thread.js`: contains bootstrappers
    that can be run in all threads, including:
  - `setupAssert` for `process.assert`
  - `setupCpuUsage` for `process.cpuUsage`
  - `setupHrtime` for `process.hrtime` and `process.hrtime.bigint`
  - `setupMemoryUsage` for `process.memoryUsage`
  - `setupConfig` for `process.config`
  - `setupKillAndExit` for `process.kill` and `process.exit`
  - `setupRawDebug` for `process._rawDebug`
  - `setupUncaughtExceptionCapture` for
    `process.setUncaughtExceptionCaptureCallback` and
    `process.hasUncaughtExceptionCaptureCallback`

Hopefully in the future we can sort more bootstrappers in
`boostrap/node.js` into these three files and further group
them into functions that can be run before creating the
snapshot / after loading the snapshot.

This patch also moves most of the `isMainThread` conditionals
into the main bootstrapper instead of letting them scattered around
special-casing different implementations.

PR-URL: nodejs#21378
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
targos pushed a commit that referenced this pull request Jul 18, 2018
This patch split part of the bootstrappers into three files:

- `lib/internal/process/main_thread_only.js`: contains bootstrappers
  that can only be run in the main thread, including
  - `setupStdio` for the main thread that sets up `process.stdin`,
    `process.stdout`, `process.error` that may interact with external
    resources, e.g. TTY/File/Pipe/TCP sockets
  - `setupProcessMethods` that setup methods changing process-global
    states, e.g. `process.chdir`, `process.umask`, `process.setuid`
  - `setupSignalHandlers`
  - `setupChildProcessIpcChannel` that setup `process.send` for
    child processes.
- `lib/internal/process/worker_thread_only.js`: contains bootstrappers
  that can only be run in the worker threads, including
  - `setupStdio` for the worker thread that are streams to be
    manipulated or piped to the parent thread
- `lib/internal/process/per_thread.js`: contains bootstrappers
    that can be run in all threads, including:
  - `setupAssert` for `process.assert`
  - `setupCpuUsage` for `process.cpuUsage`
  - `setupHrtime` for `process.hrtime` and `process.hrtime.bigint`
  - `setupMemoryUsage` for `process.memoryUsage`
  - `setupConfig` for `process.config`
  - `setupKillAndExit` for `process.kill` and `process.exit`
  - `setupRawDebug` for `process._rawDebug`
  - `setupUncaughtExceptionCapture` for
    `process.setUncaughtExceptionCaptureCallback` and
    `process.hasUncaughtExceptionCaptureCallback`

Hopefully in the future we can sort more bootstrappers in
`boostrap/node.js` into these three files and further group
them into functions that can be run before creating the
snapshot / after loading the snapshot.

This patch also moves most of the `isMainThread` conditionals
into the main bootstrapper instead of letting them scattered around
special-casing different implementations.

PR-URL: #21378
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>

Backport-PR-URL: #21866
Reviewed-By: Michaël Zasso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. process Issues and PRs related to the process subsystem. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants