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

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

Closed
wants to merge 3 commits into from

Conversation

@joyeecheung
Copy link
Member

commented Nov 21, 2018

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
console: lazy load process.stderr and process.stdout
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.
@nodejs-github-bot

This comment has been minimized.

Copy link

commented Nov 21, 2018

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

@joyeecheung

This comment has been minimized.

@@ -11,4 +11,4 @@ const list = process.moduleLoadList.slice();

const assert = require('assert');

assert(list.length <= 78, list);
assert(list.length <= 56, list);

This comment has been minimized.

Copy link
@devsnek
@@ -1,2 +1,2 @@
calling stdout._refreshSize

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Nov 21, 2018

Author Member

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.

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2018

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

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

This comment has been minimized.

Copy link
@richardlau

richardlau Nov 21, 2018

Member

common.isMainThread?

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Nov 22, 2018

Author Member

@richardlau Didn't know about this, thanks!

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Nov 22, 2018

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/

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Nov 27, 2018

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

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2018

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2018

Landed in 3337836, thanks!

joyeecheung added a commit that referenced this pull request Nov 28, 2018
console: lazy load process.stderr and process.stdout
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>
@BridgeAR

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

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

@targos targos added this to Backport requested in v11.x Dec 7, 2018

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

@targos

This comment has been minimized.

Copy link
Member

commented Jan 1, 2019

@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.

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jan 1, 2019

@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..

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

@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.

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

#24047 needs to be backported together as well

joyeecheung added a commit to joyeecheung/node that referenced this pull request Jan 9, 2019
console: lazy load process.stderr and process.stdout
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>
@joyeecheung joyeecheung referenced this pull request Jan 9, 2019
2 of 2 tasks complete
BridgeAR added a commit that referenced this pull request Jan 9, 2019
console: lazy load process.stderr and process.stdout
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>

@BridgeAR BridgeAR moved this from Backport requested to Backported in v11.x Jan 10, 2019

refack added a commit to refack/node that referenced this pull request Jan 14, 2019
console: lazy load process.stderr and process.stdout
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>
@BridgeAR BridgeAR referenced this pull request Jan 16, 2019
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
console: lazy load process.stderr and process.stdout
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>
@MylesBorins MylesBorins referenced this pull request Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
7 participants
You can’t perform that action at this time.