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: refactor to use rest params and template strings #6233

Closed
wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Apr 15, 2016

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

@jasnell jasnell added the console Issues and PRs related to the console subsystem. label Apr 15, 2016
@mscdex
Copy link
Contributor

mscdex commented Apr 15, 2016

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

@jasnell
Copy link
Member Author

jasnell commented Apr 15, 2016

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

@jasnell
Copy link
Member Author

jasnell commented Apr 15, 2016

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

@mscdex
Copy link
Contributor

mscdex commented Apr 15, 2016

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

@jasnell
Copy link
Member Author

jasnell commented Apr 15, 2016

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

@Fishrock123
Copy link
Contributor

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.

@jasnell
Copy link
Member Author

jasnell commented Apr 19, 2016

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

@Fishrock123
Copy link
Contributor

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
```
@jasnell
Copy link
Member Author

jasnell commented Apr 19, 2016

@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

@jasnell
Copy link
Member Author

jasnell commented Apr 20, 2016

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

@mscdex
Copy link
Contributor

mscdex commented Apr 20, 2016

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

@jasnell
Copy link
Member Author

jasnell commented Apr 20, 2016

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

@mscdex
Copy link
Contributor

mscdex commented Apr 20, 2016

LGTM

jasnell added a commit that referenced this pull request Apr 20, 2016
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 <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Apr 20, 2016

Landed in 33c242e

@jasnell jasnell closed this Apr 20, 2016
Trott added a commit to Trott/io.js that referenced this pull request Apr 20, 2016
It appears that a lint error was accidentally committed to master. This
fixes the issue.

Refs: nodejs#6233
@Trott
Copy link
Member

Trott commented Apr 20, 2016

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

@MylesBorins
Copy link
Contributor

already fixed in cff2a13

@jasnell
Copy link
Member Author

jasnell commented Apr 20, 2016

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

@jasnell
Copy link
Member Author

jasnell commented Apr 20, 2016

@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

@Trott
Copy link
Member

Trott commented Apr 21, 2016

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

@jasnell
Copy link
Member Author

jasnell commented Apr 21, 2016

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.

joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
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 <[email protected]>
jasnell added a commit that referenced this pull request Apr 26, 2016
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 <[email protected]>
ryzokuken added a commit to ryzokuken/node that referenced this pull request Mar 29, 2018
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
lpinca pushed a commit that referenced this pull request Apr 1, 2018
- 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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Apr 2, 2018
- 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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 4, 2018
- 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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
console Issues and PRs related to the console subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants