console: lazy load process.stderr and process.stdout by joyeecheung · Pull Request #24534 · nodejs/node · GitHub

This patch:

Refactors the Console constructor: moves the property binding code
and the writable streams binding code into two methods defined
on the Console.prototype with symbols.
Refactors the global console creation: we only need to share the
property binding code from the Console constructor. To bind the
streams we can lazy load process.stdio and process.stderr
so that we don't create these streams when they are not used.
This significantly reduces the number of modules loaded during
bootstrap. Also, by calling the refactored-out method directly
we can skip the unnecessary typechecks when creating the global
console and there is no need to create a temporary Console
anymore.
Refactors the error handler creation and the write method:
use a kUseStdout symbol to tell the internals which stream
should be loaded from the console instance. Also put the
write method on the Console prototype so it just loads
other properties directly off the console instance which simplifies
the call sites.

Also leaves a few TODOs for further refactoring of the console
bootstrap.

Local benchmark results

                                                                                   confidence improvement accuracy (*)   (**)  (***)
 misc/startup.js mode='process' script='benchmark/fixtures/require-cachable' dur=1                -0.89 %       ±1.65% ±2.19% ±2.86%
 misc/startup.js mode='process' script='test/fixtures/semicolon' dur=1                    ***      9.94 %       ±1.48% ±1.97% ±2.57%
 misc/startup.js mode='worker' script='benchmark/fixtures/require-cachable' dur=1                  0.32 %       ±2.34% ±3.11% ±4.05%
 misc/startup.js mode='worker' script='test/fixtures/semicolon' dur=1                             -0.41 %       ±2.12% ±2.83% ±3.69%

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

This patch: - Refactors the Console constructor: moves the property binding code into and the writable streams binding code into two methods defined on the Console.prototype with symbols. - Refactors the global console creation: we only need to share the property binding code from the Console constructor. To bind the streams we can lazy load `process.stdio` and `process.stderr` so that we don't create these streams when they are not used. This significantly reduces the number of modules loaded during bootstrap. Also, by calling the refactored-out method directly we can skip the unnecessary typechecks when creating the global console and there is no need to create a temporary Console anymore. - Refactors the error handler creation and the `write` method: use a `kUseStdout` symbol to tell the internals which stream should be loaded from the console instance. Also put the `write` method on the Console prototype so it just loads other properties directly off the console instance which simplifies the call sites. Also leaves a few TODOs for further refactoring of the console bootstrap.

@joyeecheung sadly an error occured when I tried to trigger a build :(

CI: https://ci.nodejs.org/job/node-test-pull-request/18827/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/278/

cc @nodejs/process

CITGM on master: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1639/
CITGM on PR branch: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1640/

cc @SimenB

😅

The order changed here because in the test test-stderr-stdout-handle-sigwinch, process.stderr is accessed before process.stdout so the SIGWINCH even listeners are attached in this order. I don't think the order by which we refresh the size the two streams makes a difference, though.

BTW I think this somewhat eliminates #17058 (comment) (at least for non-workers), since these are now gone from the process.moduleLoadList computed right after bootstrap:

Binding tty_wrap,
Binding stream_wrap,
Binding tcp_wrap,
Binding pipe_wrap

cc @hashseed @liqyan

common.isMainThread?

@richardlau Didn't know about this, thanks!

Fixed the test for the worker CI - they need to use the streams to communicate with main thread anyways so still need to load 78 modules after bootstrap.

@richardlau Thanks for the suggestion, updated.

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

I plan to land this after the CI is unlocked and have another fresh CI run.

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

Landed in 3337836, thanks!

This patch: - Refactors the Console constructor: moves the property binding code into and the writable streams binding code into two methods defined on the Console.prototype with symbols. - Refactors the global console creation: we only need to share the property binding code from the Console constructor. To bind the streams we can lazy load `process.stdio` and `process.stderr` so that we don't create these streams when they are not used. This significantly reduces the number of modules loaded during bootstrap. Also, by calling the refactored-out method directly we can skip the unnecessary typechecks when creating the global console and there is no need to create a temporary Console anymore. - Refactors the error handler creation and the `write` method: use a `kUseStdout` symbol to tell the internals which stream should be loaded from the console instance. Also put the `write` method on the Console prototype so it just loads other properties directly off the console instance which simplifies the call sites. Also leaves a few TODOs for further refactoring of the console bootstrap. PR-URL: #24534 Reviewed-By: Gus Caplan <me@gus.host>

This does not land cleanly on 11.x. Please open a backport or add a label if this should not be backported.

Ping @joyeecheung

@joyeecheung would you like to backport this PR to 11.x? It seems critical if we want to keep the branch up to date with all other subsequent refactorings that happened on master.

@targos This depends on #23509 which is unfortunately semver-major...maybe it can be "backported" only for the sake of resolving conflicts but somehow maintain the previous behavior..

@joyeecheung that would be awesome. It would help a lot of the non breaking changes could be backported. It's getting pretty hard to prepare a release at the moment due to many conflicts.

#24047 needs to be backported together as well

This patch: - Refactors the Console constructor: moves the property binding code into and the writable streams binding code into two methods defined on the Console.prototype with symbols. - Refactors the global console creation: we only need to share the property binding code from the Console constructor. To bind the streams we can lazy load `process.stdio` and `process.stderr` so that we don't create these streams when they are not used. This significantly reduces the number of modules loaded during bootstrap. Also, by calling the refactored-out method directly we can skip the unnecessary typechecks when creating the global console and there is no need to create a temporary Console anymore. - Refactors the error handler creation and the `write` method: use a `kUseStdout` symbol to tell the internals which stream should be loaded from the console instance. Also put the `write` method on the Console prototype so it just loads other properties directly off the console instance which simplifies the call sites. Also leaves a few TODOs for further refactoring of the console bootstrap. PR-URL: nodejs#24534 Reviewed-By: Gus Caplan <me@gus.host>

This patch: - Refactors the Console constructor: moves the property binding code into and the writable streams binding code into two methods defined on the Console.prototype with symbols. - Refactors the global console creation: we only need to share the property binding code from the Console constructor. To bind the streams we can lazy load `process.stdio` and `process.stderr` so that we don't create these streams when they are not used. This significantly reduces the number of modules loaded during bootstrap. Also, by calling the refactored-out method directly we can skip the unnecessary typechecks when creating the global console and there is no need to create a temporary Console anymore. - Refactors the error handler creation and the `write` method: use a `kUseStdout` symbol to tell the internals which stream should be loaded from the console instance. Also put the `write` method on the Console prototype so it just loads other properties directly off the console instance which simplifies the call sites. Also leaves a few TODOs for further refactoring of the console bootstrap. PR-URL: #24534 Reviewed-By: Gus Caplan <me@gus.host>

This patch: - Refactors the Console constructor: moves the property binding code into and the writable streams binding code into two methods defined on the Console.prototype with symbols. - Refactors the global console creation: we only need to share the property binding code from the Console constructor. To bind the streams we can lazy load `process.stdio` and `process.stderr` so that we don't create these streams when they are not used. This significantly reduces the number of modules loaded during bootstrap. Also, by calling the refactored-out method directly we can skip the unnecessary typechecks when creating the global console and there is no need to create a temporary Console anymore. - Refactors the error handler creation and the `write` method: use a `kUseStdout` symbol to tell the internals which stream should be loaded from the console instance. Also put the `write` method on the Console prototype so it just loads other properties directly off the console instance which simplifies the call sites. Also leaves a few TODOs for further refactoring of the console bootstrap. PR-URL: nodejs#24534 Reviewed-By: Gus Caplan <me@gus.host>

nodejs-github-bot added the console Issues and PRs related to the console subsystem. label Nov 21, 2018

devsnek approved these changes Nov 21, 2018

View reviewed changes

joyeecheung commented Nov 21, 2018

View reviewed changes

fixup! console: lazy load process.stderr and process.stdout

d422bff

richardlau reviewed Nov 21, 2018

View reviewed changes

fixup! fixup! console: lazy load process.stderr and process.stdout

e93b4b9

joyeecheung added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 28, 2018

joyeecheung closed this Nov 28, 2018

BridgeAR added the backport-requested-v11.x label Dec 5, 2018

joyeecheung mentioned this pull request Jan 9, 2019

[v11.x] backport console related changes #25420

Closed

2 tasks

BridgeAR added backported-to-v11.x and removed backport-requested-v11.x labels Jan 9, 2019

BridgeAR mentioned this pull request Jan 16, 2019

v11.7.0 proposal #25537

Merged

MylesBorins mentioned this pull request Jan 24, 2019

v11.8.0 proposal #25687

Merged

BethGriggs added the backport-requested-v10.x label Jun 25, 2019

+              // to eliminate the noise introduced by --experimental-worker
+              const { internalBinding } = require('internal/test/binding');
+              const { threadId } = internalBinding('worker');
+              const isMainThread = threadId === 0;

               const assert = require('assert');
-              assert(list.length <= 78, list);
+              assert(list.length <= 56, list);

		@@ -1,2 +1,2 @@
		calling stdout._refreshSize

console: lazy load process.stderr and process.stdout by joyeecheung · Pull Request #24534 · nodejs/node · GitHub

Uh oh!

Uh oh!

console: lazy load process.stderr and process.stdout - #24534

console: lazy load process.stderr and process.stdout#24534joyeecheung wants to merge 3 commits intonodejs:masternodejs/node:masterfrom joyeecheung:lazy-consolejoyeecheung/node:lazy-consoleCopy head branch name to clipboard

Conversation

joyeecheung commented Nov 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Nov 21, 2018

Uh oh!

joyeecheung commented Nov 21, 2018

Uh oh!

joyeecheung commented Nov 21, 2018

Uh oh!

devsnek Nov 21, 2018

Choose a reason for hiding this comment

Uh oh!

joyeecheung Nov 21, 2018 • 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!

joyeecheung commented Nov 21, 2018

Uh oh!

richardlau Nov 21, 2018

Choose a reason for hiding this comment

Uh oh!

joyeecheung Nov 22, 2018

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Nov 22, 2018

Uh oh!

joyeecheung commented Nov 27, 2018

Uh oh!

joyeecheung commented Nov 28, 2018

Uh oh!

joyeecheung commented Nov 28, 2018

Uh oh!

BridgeAR commented Dec 5, 2018

Uh oh!

BridgeAR commented Dec 11, 2018

Uh oh!

targos commented Jan 1, 2019

Uh oh!

joyeecheung commented Jan 1, 2019

Uh oh!

BridgeAR commented Jan 9, 2019

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

console: lazy load process.stderr and process.stdout#24534
joyeecheung wants to merge 3 commits into
nodejs:masterfrom
joyeecheung:lazy-console

joyeecheung commented Nov 21, 2018 •
edited
Loading

joyeecheung Nov 21, 2018 •
edited
Loading

joyeecheung commented Jan 9, 2019 •
edited
Loading