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

doc: add caveat and tradeoff example to readline #26472

Closed
wants to merge 1 commit into from
Closed

doc: add caveat and tradeoff example to readline #26472

wants to merge 1 commit into from

Conversation

vsemozhetbyt
Copy link
Contributor

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

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

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

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

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
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
PR-URL: nodejs#26472
Refs: nodejs#23916
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
BridgeAR pushed a commit that referenced this pull request Mar 14, 2019
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
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
@Palisand
Copy link

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

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

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

Successfully merging this pull request may close these issues.

5 participants