readline: add support for async iteration

[TimothyGu]

Rewritten version of #18904, using more existing streams mechanisms.

Depends on #23901 for some of the edge case tests (relevant commits included within this PR).

Co-authored-by: Ivan Filenko ivan.filenko@protonmail.com
Fixes: #18603
Refs: #18904

/cc @mcollina @devsnek @prog1dev

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

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

[TimothyGu]

CI: https://ci.nodejs.org/job/node-test-pull-request/18180/

[vsemozhetbyt]

Should we benchmark this comparatively with the 'line' event way and maybe add a note if this currently proves to be significantly slower? (Refs: #23032 (comment))

[mcollina]

Good work!

[mcollina]

Would it make sense expose this as a separate method? converting to a stream might be an issue for multiple people.

[TimothyGu]

I consider this an implementation detail of @@asyncIterator method. A major reason of why the performance of this method isn't up to par to 'line' event, as you have noted in #23916 (comment), is because of the double buffering necessitated by the intermediate stream, so I'd rather not expose the stream at the moment.

[vsemozhetbyt]

I've compiled the branch and tested with 1 GB file (22 514 395 lines):

Scripts and results:

'use strict';

const fs = require('fs');
const readline = require('readline');

let counter = 0;
let dummy;

console.time('event');

const rl = readline.createInterface({
  input: fs.createReadStream('big-file.txt', 'utf8'),
  crlfDelay: Infinity,
});

rl.on('line', (line) => {
  counter++;
  dummy = line;
}).on('close', () => {
  console.timeEnd('event');
  console.log(`Lines: ${counter}, last line length: ${dummy.length}`);
});

event: 15763.332ms
Lines: 22514395, last line length: 1

'use strict';

const fs = require('fs');
const readline = require('readline');

(async function main() {
  let counter = 0;
  let dummy;

  console.time('asyncIterator');

  const rl = readline.createInterface({
    input: fs.createReadStream('big-file.txt', 'utf8'),
    crlfDelay: Infinity,
  });

  for await (const line of rl) {
    counter++;
    dummy = line;
  }

  console.timeEnd('asyncIterator');
  console.log(`Lines: ${counter}, last line length: ${dummy.length}`);
})();

asyncIterator: 44865.388ms
Lines: 22514395, last line length: 1

So event implementation currently 3 times as fast as async iterator implementation. Maybe we should warn about this.

[mcollina]

Can you check reading the same file with fs.createReadStream()  and iterating? I think our last check was about 2 times slower. Considering that we are doing double-buffering, a 3x factor seems ok to me. I don't know if we can improve this at all, a good part of that is due to async iteration overhead.

[vsemozhetbyt]

@mcollina Do you mean to compare async iterating over unsplit chunks vs async iterating over split lines? If so, I have ~ 4x factor:

Scripts and results:

const fs = require('fs');

(async function main() {
  let counter = 0;
  let dummy;

  console.time('asyncIteratorChunks');

  for await (const chunk of fs.createReadStream('big-file.txt', 'utf8')) {
    counter++;
    dummy = chunk;
  }

  console.timeEnd('asyncIteratorChunks');
  console.log(`Chunks: ${counter}, last chunk length: ${dummy.length}`);
})();

asyncIteratorChunks: 10048.069ms
Chunks: 15811, last chunk length: 22280

'use strict';

const fs = require('fs');
const readline = require('readline');

(async function main() {
  let counter = 0;
  let dummy;

  console.time('asyncIteratorLines');

  const rl = readline.createInterface({
    input: fs.createReadStream('big-file.txt', 'utf8'),
    crlfDelay: Infinity,
  });

  for await (const line of rl) {
    counter++;
    dummy = line;
  }

  console.timeEnd('asyncIteratorLines');
  console.log(`Lines: ${counter}, last line length: ${dummy.length}`);
})();

asyncIteratorLines: 43501.427ms
Lines: 22514395, last line length: 1

[vsemozhetbyt]

@mcollina Or do you mean to compare async iterating over unsplit chunks vs event implementation for unsplit chunks? If so, I have 1:1 factor, i.e. the same speed:

Scripts and results:

'use strict';

const fs = require('fs');

let counter = 0;
let dummy;

console.time('eventChunks');

const readable = fs.createReadStream('big-file.txt', 'utf8');

readable.on('data', (chunk) => {
  counter++;
  dummy = chunk;
}).on('close', () => {
  console.timeEnd('eventChunks');
  console.log(`Chunks: ${counter}, last chunk length: ${dummy.length}`);
});

eventChunks: 9978.033ms
Chunks: 15811, last chunk length: 22280

'use strict';

const fs = require('fs');

(async function main() {
  let counter = 0;
  let dummy;

  console.time('asyncIteratorChunks');

  for await (const chunk of fs.createReadStream('big-file.txt', 'utf8')) {
    counter++;
    dummy = chunk;
  }

  console.timeEnd('asyncIteratorChunks');
  console.log(`Chunks: ${counter}, last chunk length: ${dummy.length}`);
})();

asyncIteratorChunks: 9922.896ms
Chunks: 15811, last chunk length: 22280

[vsemozhetbyt]

#23901 has landed, it seems those commits can be excluded to simplify reviews.

[vsemozhetbyt]

And beware #23929, we may have conflicts.

[mcollina]

@vsemozhetbyt thanks for those benchmarks, those are quite interesting. Specifically the fact that using stream iteration is now essentially on par with on('data') is fantastic!

Related to 'readline', I think the culprit of the problem is double-buffering. Essentially we would need a custom implementation that does not rely on the streams one if we want to improve it.

[TimothyGu]

@vsemozhetbyt @mcollina I've updated this PR to address the documentation comments. Please take a look.

[mcollina]

LGTM

[vsemozhetbyt]

Docs LGTM. Thank you!

[vsemozhetbyt]

Maybe cc @nodejs/streams ?

[mcollina]

@devsnek maybe? Anyway this could land because it's older than a week. I'd recommend to wait for 2 days and then land if no one objects.

[benjamingr]

Looking at this now (to solve another bug) I think this backpressure behaviour is confusing since other consumers can listen to line on the stream and it's surprising we pause() it for them.