GeezerVison

Review requested:

• @nodejs/performance
• @nodejs/streams

Codecov Report

❌ Patch coverage is 92.99065% with 45 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.15%. Comparing base (9ad097b) to head (df55497).
⚠️ Report is 249 commits behind head on main.

@@            Coverage Diff             @@
##             main   #64429      +/-   ##
==========================================
- Coverage   90.33%   90.15%   -0.19%     
==========================================
  Files         751      751              
  Lines      250341   254051    +3710     
  Branches    47322    47897     +575     
==========================================
+ Hits       226145   229037    +2892     
- Misses      15575    16276     +701     
- Partials     8621     8738     +117     

I'm seeing #64447, which may change the measured performance of the current implementation.

macbook-stream-operator-throughput-report.pdf
wsl2-stream-operator-throughput-report.pdf
linux-server-stream-operator-throughput-report.pdf
I've run the performance comparisons again with the latest changes from the main branch. macbook is the same M2 Max MacBook as before, linux-server is a 10900KF Intel CPU running on Debian 13, and wsl2 is a Windows machine with an AMD 9900x processor, and the tests ran within a WSL2 with Ubuntu 24.04

@nodejs/streams I know you guys have probably 20 million PRs to review but if you could add this one to the queue I'd really appreciate it :)

The changes on streams/operators are quite significant, making the review of the code much harder. It'd be helpful if you could reorganize your changes in a way that minimizes diff. Or decouple current behavior from the added behavior of "data already being available"

My main concern is due to the nature of the diff, it is hard to see/visualize what actually was changed in terms of behavior/logic versus what is just moving code around. I'm not an expert on the streams implementation within node, but possibly my peers would argue the same.

Do you believe there any improvements you can do to the current diff?

@ovflowd thanks for taking a look at the code. I could split the changes in from.js and the updated tests into a different PR, if that would help minimize the amount of code changes in this one.

In terms of the logic inside operators.js, it's not easy to view this as a diff because the previous logic almost entirely relied on createAsyncIterator which is in a different file readable.js and the asynchronous iteration engine that v8 provides. The new proposal is an implementation from scratch that keeps the same compatibility evaluated by the tests, but with improved performance. It's better to see both as black boxes implementations rather than trying to diff.

I could also split the changes into a few sections (maybe different commits?):

Functions that return a stream:

• changes to drop, map, filter (drop depends on filter and filter depends on map).

I reiterate that flatMap and take have not been changed by this PR.

Functions that return a value:

• changes to forEach, some, every, find. (the first two rely on the last)
• changes to reduce
• changes to toArray

I have split the changes into well-defined commits that should ease the review of each affected function.

I'm going to close this one to open smaller PRs with each commit.