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.

Sign up for GitHub

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

Jump to bottom

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
Copy link
Collaborator

@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

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

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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Merged
@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.

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

@BridgeAR BridgeAR BridgeAR approved these changes

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