Skip to content

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-botnodejs-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
@vsemozhetbytvsemozhetbyt 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
ContributorAuthor

vsemozhetbyt commented Mar 6, 2019

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

@BridgeAR
Copy link
Member

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

@vsemozhetbyt
Copy link
ContributorAuthor

See #23916 (comment) for a simple comparing:

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

@BridgeAR
Copy link
Member

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

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.

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

@mcollinamcollina 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
ContributorAuthor

Landed in 15e741a
Thank you for the reviews.

@vsemozhetbytvsemozhetbyt 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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
BridgeAR pushed a commit that referenced this pull request Mar 14, 2019
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
PR-URL: #26472 Refs: #23916 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
@BethGriggsBethGriggs 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
ContributorAuthor

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

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

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