★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/26472Nouvelle recherche | Portail wanayoo
Skip to content

/node

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
base: master
from

Conversation

Reviewers

@mcollina mcollina

@BridgeAR BridgeAR

Assignees
No one assigned
Labels
author ready doc events performance promises readline
Projects
None yet
Milestone
No milestone
4 participants
@vsemozhetbyt @nodejs-github-bot @BridgeAR @mcollina
@vsemozhetbyt
Copy link
Member

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

@nodejs-github-bot nodejs-github-bot added doc readline labels Mar 6, 2019

@vsemozhetbyt vsemozhetbyt added events performance promises labels Mar 6, 2019

@vsemozhetbyt

This comment has been minimized.

Sign in to view
Copy link
Member Author

vsemozhetbyt commented Mar 6, 2019 •
edited

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

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
Copy link
Member 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

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
Copy link
Member Author

vsemozhetbyt commented Mar 6, 2019 •
edited

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 approved these changes Mar 6, 2019
View changes

@BridgeAR BridgeAR added the author ready label Mar 6, 2019

@mcollina

mcollina approved these changes Mar 7, 2019
View changes

Copy link
Member

mcollina left a comment

LGTM
@vsemozhetbyt

This comment has been minimized.

Sign in to view
Copy link
Member Author

vsemozhetbyt commented Mar 8, 2019

Landed in 15e741a
Thank you for the reviews.

@vsemozhetbyt vsemozhetbyt closed this Mar 8, 2019

@vsemozhetbyt vsemozhetbyt deleted the vsemozhetbyt:doc-readline-once branch Mar 8, 2019

vsemozhetbyt added a commit that referenced this pull request Mar 8, 2019

@vsemozhetbyt
doc: add caveat and tradeoff example to readline 
PR-URL: #26472
Refs: #23916
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Loading status checks…
15e741a

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 12, 2019

@vsemozhetbyt @BridgeAR
doc: add caveat and tradeoff example to readline 
PR-URL: nodejs#26472
Refs: nodejs#23916
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Verified
This commit was signed with a verified signature.
@BridgeAR BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
38f8d2a

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 13, 2019

@vsemozhetbyt @BridgeAR
doc: add caveat and tradeoff example to readline 
PR-URL: nodejs#26472
Refs: nodejs#23916
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Verified
This commit was signed with a verified signature.
@BridgeAR BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
bab6c63

@BridgeAR BridgeAR referenced this pull request Mar 13, 2019

Merged

2019-03-14, Version 11.12.0 (Current) #26644

BridgeAR added a commit that referenced this pull request Mar 14, 2019

@vsemozhetbyt @BridgeAR
doc: add caveat and tradeoff example to readline 
PR-URL: #26472
Refs: #23916
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Verified
This commit was signed with a verified signature.
@BridgeAR BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
2ee9a96
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.