Skip to content

doc: add caveat and tradeoff example to readline#26472

Closed
vsemozhetbyt wants to merge 1 commit intonodejs:masterfrom
vsemozhetbyt:doc-readline-once
Closed

doc: add caveat and tradeoff example to readline#26472
vsemozhetbyt wants to merge 1 commit intonodejs:masterfrom
vsemozhetbyt:doc-readline-once

Conversation

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Mar 6, 2019

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

Refs: #23916

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 6, 2019

@vsemozhetbyt build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2782/pipeline

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. readline Issues and PRs related to the built-in readline module. labels Mar 6, 2019
@vsemozhetbyt vsemozhetbyt added events Issues and PRs related to the events subsystem / EventEmitter. performance Issues and PRs related to the performance of Node.js. promises Issues and PRs related to ECMAScript promises. labels Mar 6, 2019
@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Mar 6, 2019 •
edited
Loading

If this is approved, this should be backported after backporting of #26078.

@BridgeAR
Copy link
Member

BridgeAR commented Mar 6, 2019

Is the performance for readline something critical? If so, I'll have a look at improving it.

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Mar 6, 2019

See #23916 (comment) for a simple comparing:

So event implementation currently 3 times as fast as async iterator implementation.

@BridgeAR
Copy link
Member

BridgeAR commented Mar 6, 2019

@vsemozhetbyt it's not about the actual performance. I am pretty sure there is indeed a significant performance difference. I just wonder if that matters at all for the readline module. I currently can not think of any use case that is performance critical while using readline.

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Mar 6, 2019 •
edited
Loading

Say, I have a huge text file in some parsable format (CSV, digital dictionary, logfile etc). I want to process it line by line, retrieve some data or convert it in another format. Currently, readline is the only core way to do this. For files about 1 GB, the difference between 15 and 45 sec seems significant, especially if I have a batch of such files to process.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 6, 2019
mcollina
mcollina approved these changes Mar 7, 2019
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Mar 8, 2019

Landed in 15e741a
Thank you for the reviews.

@vsemozhetbyt vsemozhetbyt deleted the doc-readline-once branch March 8, 2019 16:10
vsemozhetbyt added a commit that referenced this pull request Mar 8, 2019
@vsemozhetbyt
doc: add caveat and tradeoff example to readline
15e741a 
PR-URL: #26472
Refs: #23916
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 13, 2019
@vsemozhetbyt @BridgeAR
doc: add caveat and tradeoff example to readline
bab6c63 
PR-URL: nodejs#26472
Refs: nodejs#23916
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Mar 13, 2019
Merged
BridgeAR pushed a commit that referenced this pull request Mar 14, 2019
@vsemozhetbyt @BridgeAR
doc: add caveat and tradeoff example to readline
2ee9a96 
PR-URL: #26472
Refs: #23916
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
BethGriggs pushed a commit that referenced this pull request Apr 16, 2019
@vsemozhetbyt @BethGriggs
doc: add caveat and tradeoff example to readline
4065785 
PR-URL: #26472
Refs: #23916
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Merged
@Palisand
Copy link

Palisand commented May 19, 2019

I cannot get this to work. An error is thrown during the invocation of once: TypeError: once is not a function.

(v11.11.0)

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented May 19, 2019

@Palisand As per "Added in:" field in the doc, you need v11.13.0 at least.

This was referenced May 29, 2019
Closed
Closed
Closed
This was referenced May 29, 2019
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. events Issues and PRs related to the events subsystem / EventEmitter. performance Issues and PRs related to the performance of Node.js. promises Issues and PRs related to ECMAScript promises. readline Issues and PRs related to the built-in readline module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vsemozhetbyt @nodejs-github-bot @BridgeAR @Palisand @mcollina