console: refactor to use rest params and template strings by jasnell · Pull Request #6233 · nodejs/node · GitHub

Checklist

tests and code linting passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

console

Description of change

Overall cleanup in code, eliminate reliance on arguments.

Benchmarks show that as of v8 5.0.71.32, using rest params + apply has good performance. The spread operator is not yet well optimized in v8

misc/console.js method=restAndSpread concat=1 n=1000000: 374779.38359
misc/console.js method=restAndSpread concat=0 n=1000000: 375988.30434
misc/console.js method=argumentsAndApply concat=1 n=1000000: 682618.61125
misc/console.js method=argumentsAndApply concat=0 n=1000000: 645093.74443
misc/console.js method=restAndApply concat=1 n=1000000: 682931.41217
misc/console.js method=restAndApply concat=0 n=1000000: 664473.09700

It would be nice to see manual string concatenation added to the benchmarks. I suspect that would be the "fastest" (at least compared to template strings the last I checked).

It's there.. see https://github.com/nodejs/node/pull/6233/files#diff-32494b677ab738804f3e70d72fa804adR31 .. specifically, the concat=1 config above means doing manual concat whereas concat=0 means use template strings

And you're right, they edge out template strings just a bit... but not by much.

@jasnell There is only benchmarking of util.format() and template strings currently. I meant also adding benchmarks for testing raw concatenation like 'this is ' + arguments[0] + ' of ' + arguments[1].

oh! misunderstood... I was referring to the concat in util.format(...args) + '\n'.

It may be good to leave these (but not the benchmarks) for new contributors to deal with, since it's pretty easy to instruct people to change stuff to use es6.

Why? I'm working in code already anyway (#6174). Is there a reason not to land this PR?

Why?

since it's pretty easy to instruct people to change stuff to use es6.

Is there a reason not to land this PR?

I meant the type of changes, as exciting as it may be for us. I haven't checked this PR, but so long as perf is good we should be fine I suppose.

Overall cleanup in code, eliminate reliance on `arguments`. Benchmarks show that as of v8 5.0.71.32, using rest params + apply has good performance. The spread operator is not yet well optimized in v8 ``` misc/console.js method=restAndSpread concat=1 n=1000000: 374779.38359 misc/console.js method=restAndSpread concat=0 n=1000000: 375988.30434 misc/console.js method=argumentsAndApply concat=1 n=1000000: 682618.61125 misc/console.js method=argumentsAndApply concat=0 n=1000000: 645093.74443 misc/console.js method=restAndApply concat=1 n=1000000: 682931.41217 misc/console.js method=restAndApply concat=0 n=1000000: 664473.09700 ```

@mscdex ... updated the benchmark to include a rest params + concat only example and you're right, it is the fastest approach, but not by a whole lot. Using rest params with apply, util.format and concat for the \n seems to be a reasonable choice here.

misc/console.js method=restAndSpread concat=1 n=1000000: 431909.61616
misc/console.js method=restAndSpread concat=0 n=1000000: 380620.89756
misc/console.js method=argumentsAndApply concat=1 n=1000000: 695168.75332
misc/console.js method=argumentsAndApply concat=0 n=1000000: 720588.97836
misc/console.js method=restAndApply concat=1 n=1000000: 726704.47661
misc/console.js method=restAndApply concat=0 n=1000000: 725573.25529
misc/console.js method=restAndConcat concat=1 n=1000000: 783515.12632

@mscdex ... how does this look to you now?

What is the reason for using a stream in the benchmarks?

@mscdex ... because I didn't want to write out to console but still wanted to keep the scenario as close to console.log and console.warn as possible.

LGTM

Overall cleanup in code, eliminate reliance on `arguments`. Benchmarks show that as of v8 5.0.71.32, using rest params + apply has good performance. The spread operator is not yet well optimized in v8 ``` misc/console.js method=restAndSpread concat=1 n=1000000: 374779.38359 misc/console.js method=restAndSpread concat=0 n=1000000: 375988.30434 misc/console.js method=argumentsAndApply concat=1 n=1000000: 682618.61125 misc/console.js method=argumentsAndApply concat=0 n=1000000: 645093.74443 misc/console.js method=restAndApply concat=1 n=1000000: 682931.41217 misc/console.js method=restAndApply concat=0 n=1000000: 664473.09700 ``` PR-URL: #6233 Reviewed-By: Brian White <mscdex@mscdex.net>

Landed in 33c242e

It appears that a lint error was accidentally committed to master. This fixes the issue. Refs: nodejs#6233

Looks like this landed without running through CI. There's a linting error. Fix is in #6315

already fixed in cff2a13

It appears I have a bug in my landing script :-/ ... sometimes it runs lint, other times it doesn't. Not quite sure why. Ah well, that's what I get for making the landing script overly complicated ;-) ... looks like I'm back to manual linting

@Trott ... what would you think about a git pre-commit hook that enforces linting before commit? Obviously folks would have to install it locally to use it but having it available in tree would be worthwhile

@jasnell Not sure. People are supposed to run make test before submitting a PR and that will run the linting, so... ¯_(ツ)_/¯

On the other hand, tools over rules and all that.

heh... well, I pulled the faulty make lint stuff out of my landing script and went back to the tried and true pre-push git hook. That'll teach me to get fancy with it.

Overall cleanup in code, eliminate reliance on `arguments`. Benchmarks show that as of v8 5.0.71.32, using rest params + apply has good performance. The spread operator is not yet well optimized in v8 ``` misc/console.js method=restAndSpread concat=1 n=1000000: 374779.38359 misc/console.js method=restAndSpread concat=0 n=1000000: 375988.30434 misc/console.js method=argumentsAndApply concat=1 n=1000000: 682618.61125 misc/console.js method=argumentsAndApply concat=0 n=1000000: 645093.74443 misc/console.js method=restAndApply concat=1 n=1000000: 682931.41217 misc/console.js method=restAndApply concat=0 n=1000000: 664473.09700 ``` PR-URL: nodejs#6233 Reviewed-By: Brian White <mscdex@mscdex.net>

Overall cleanup in code, eliminate reliance on `arguments`. Benchmarks show that as of v8 5.0.71.32, using rest params + apply has good performance. The spread operator is not yet well optimized in v8 ``` misc/console.js method=restAndSpread concat=1 n=1000000: 374779.38359 misc/console.js method=restAndSpread concat=0 n=1000000: 375988.30434 misc/console.js method=argumentsAndApply concat=1 n=1000000: 682618.61125 misc/console.js method=argumentsAndApply concat=0 n=1000000: 645093.74443 misc/console.js method=restAndApply concat=1 n=1000000: 682931.41217 misc/console.js method=restAndApply concat=0 n=1000000: 664473.09700 ``` PR-URL: #6233 Reviewed-By: Brian White <mscdex@mscdex.net>

Rename test-fs-truncate-nodejsGH-6233 to test-fs-truncate-clear-file-zero Rename test-process-exit-nodejsGH-12322 to test-process-exit-flaky-handler Refs: nodejs#19105 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure

- Rename test-fs-truncate-GH-6233 to test-fs-truncate-clear-file-zero - Rename test-process-exit-GH-12322 to test-process-exit-handler PR-URL: #19668 Refs: #19105 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell added the console Issues and PRs related to the console subsystem. label Apr 15, 2016

jasnell mentioned this pull request Apr 15, 2016

console,process: refactoring stdout/stderr error handling #6174

Closed

4 tasks

jasnell added 2 commits April 19, 2016 11:40

Add concat to benchmark

328ae5f

jasnell force-pushed the console-update branch from 4870fb5 to 328ae5f Compare April 19, 2016 18:52

jasnell added the dont-land-on-v5.x label Apr 20, 2016

jasnell added the dont-land-on-v4.x label Apr 20, 2016

jasnell closed this Apr 20, 2016

Trott added a commit to Trott/io.js that referenced this pull request Apr 20, 2016

benchmark: fix lint error in benchmark file

f5aa2ad

It appears that a lint error was accidentally committed to master. This fixes the issue. Refs: nodejs#6233

Trott mentioned this pull request Apr 20, 2016

benchmark: fix lint error in benchmark file #6315

Closed

2 tasks

ryzokuken mentioned this pull request Mar 29, 2018

test: rename regression tests with descriptive file names (pt. 7) #19668

Closed

2 tasks

console: refactor to use rest params and template strings by jasnell · Pull Request #6233 · nodejs/node · GitHub

Uh oh!

Uh oh!

console: refactor to use rest params and template strings - #6233

console: refactor to use rest params and template strings#6233jasnell wants to merge 2 commits intonodejs:masternodejs/node:masterfrom jasnell:console-updatejasnell/node:console-updateCopy head branch name to clipboard

Conversation

jasnell commented Apr 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

mscdex commented Apr 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Apr 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Apr 15, 2016

Uh oh!

mscdex commented Apr 15, 2016

Uh oh!

jasnell commented Apr 15, 2016

Uh oh!

Fishrock123 commented Apr 19, 2016

Uh oh!

jasnell commented Apr 19, 2016

Uh oh!

Fishrock123 commented Apr 19, 2016

Uh oh!

jasnell commented Apr 19, 2016

Uh oh!

jasnell commented Apr 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Apr 20, 2016

Uh oh!

jasnell commented Apr 20, 2016

Uh oh!

mscdex commented Apr 20, 2016

Uh oh!

jasnell commented Apr 20, 2016

Uh oh!

Trott commented Apr 20, 2016

Uh oh!

MylesBorins commented Apr 20, 2016

Uh oh!

jasnell commented Apr 20, 2016

Uh oh!

jasnell commented Apr 20, 2016

Uh oh!

Trott commented Apr 21, 2016

Uh oh!

jasnell commented Apr 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

console: refactor to use rest params and template strings#6233
jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:console-update

jasnell commented Apr 15, 2016 •
edited
Loading

mscdex commented Apr 15, 2016 •
edited
Loading

jasnell commented Apr 15, 2016 •
edited
Loading

jasnell commented Apr 20, 2016 •
edited
Loading