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

lib: print to stdout/stderr directly instead of using console #27320

Closed
wants to merge 2 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Apr 20, 2019

lib: print to stdout/stderr directly instead of using console

This patch adds an internal function that prints to stdout or
stderr by directly writing to the known file descriptor, and
uses it internally in common cases to avoid the overhead
of the console implementation.

benchmark: add benchmark for node -p

Local benchmark results

                                             confidence improvement accuracy (*)   (**)  (***)
 misc/print.js code='"string"' dur=1                ***     10.26 %       ±1.70% ±2.26% ±2.94%
 misc/print.js code='1' dur=1                       ***     10.92 %       ±1.46% ±1.94% ±2.52%
 misc/print.js code='process.versions' dur=1        ***      9.51 %       ±1.71% ±2.28% ±2.98%
 misc/print.js code='process' dur=1                 ***     11.34 %       ±1.71% ±2.27% ±2.96%
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

This patch adds an internal function that prints to stdout or
stderr by directly writing to the known file descriptor, and
uses it internally in common cases to avoid the overhead
of the console implementation.
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Apr 20, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

From the CI:

14:45:30                                              confidence improvement accuracy (*)   (**)  (***)
14:45:30  misc/print.js code='1' dur=1                       ***     14.64 %       ±0.64% ±0.86% ±1.11%
14:45:30  misc/print.js code='process' dur=1                 ***     14.87 %       ±0.52% ±0.69% ±0.90%
14:45:30  misc/print.js code='process.versions' dur=1        ***     14.07 %       ±0.78% ±1.04% ±1.36%
14:45:30  misc/print.js code='"string"' dur=1                ***     14.36 %       ±0.89% ±1.19% ±1.56%

@BridgeAR
Copy link
Member

If we have such significant overhead with console.log should we not try to improve the console performance instead? We have a lot of function calls for each console.log() that could instead return static values and we can probably simplify our current abstraction quite a bit.

@joyeecheung
Copy link
Member Author

joyeecheung commented Apr 21, 2019

@BridgeAR I don't think we could improve console.log() this way, specifically, the overhead comes from the stream implementation which is sort of the hidden contract of Node.js's console.log() (e.g. that it uses process.stdout). There is also a trade-off when console.log() is used to print large chunks of data - for the call sites changed here that is not likely to matter though.

@joyeecheung joyeecheung added the review wanted PRs that need reviews. label Apr 22, 2019
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

I am going to land this after the 7 day wait if no more reviews come up.

@Fishrock123
Copy link
Contributor

Fishrock123 commented Apr 26, 2019

Isn't this pretty much the same as process._rawDebug()? If so, is it necessary?

@joyeecheung
Copy link
Member Author

joyeecheung commented Apr 27, 2019

@Fishrock123 It’s not exactly the same in that the output is formatted different for compatibility (primarily colors), it handles IPC, can print to stdout, and it’s not accessible in the user land - if we change any of these to process._rawDebug() I am afraid it could be a semver-major. Also the point here is to have a lighter weight console so it needs to match the console behavior which is not so raw for process._rawDebug().

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

Landed in 31b3dd2...c5817ab

joyeecheung added a commit that referenced this pull request Apr 28, 2019
This patch adds an internal function that prints to stdout or
stderr by directly writing to the known file descriptor, and
uses it internally in common cases to avoid the overhead
of the console implementation.

PR-URL: #27320
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Apr 28, 2019
PR-URL: #27320
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 29, 2019
This patch adds an internal function that prints to stdout or
stderr by directly writing to the known file descriptor, and
uses it internally in common cases to avoid the overhead
of the console implementation.

PR-URL: #27320
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 29, 2019
PR-URL: #27320
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request May 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory. review wanted PRs that need reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants