lib: place value before done in iterator results

[bakkot]

Spec requires this order, plus V8 optimizes it and not the reverse; as such this has some performance benefits (in this microbenchmark).

cc @trivikr since you added the lint rule enforcing the opposite order in #63526.

benchmark	config	ops/sec	time per op
url/url-searchparams-iterator-consumers.js	method='Map'	+35.6% ***	−26%
url/url-searchparams-iterator-consumers.js	method='Array.from'	+24.2% ***	−19%
url/url-searchparams-iterator-consumers.js	method='Object.fromEntries'	+18.0% ***	−15%
url/url-searchparams-iterator-consumers.js	method='for-of'	−0.9% ***	+0.9%
webstreams/readable-async-iterator.js	type='normal'	+4.8% ***	−4.6%
webstreams/readable-async-iterator.js	type='bytes'	+2.2% ***	−2.1%
streams/readable-async-iterator.js	sync='yes'	+3.2% **	−3.1%
streams/readable-async-iterator.js	sync='no'	+1.4% *	−1.4%

[nodejs-github-bot]

Review requested:

• @nodejs/performance
• @nodejs/streams
• @nodejs/url

[codecov]

Codecov Report

❌ Patch coverage is 84.21053% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.49%. Comparing base (e463b48) to head (71922e3).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/webstreams/readablestream.js	85.71%	2 Missing ⚠️
lib/internal/fs/promises.js	80.00%	1 Missing ⚠️
lib/internal/source_map/source_map_cache_map.js	0.00%	1 Missing ⚠️
lib/internal/streams/readable.js	88.88%	1 Missing ⚠️
lib/internal/vfs/watcher.js	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #66837      +/-   ##
==========================================
- Coverage   92.85%   90.49%   -2.36%     
==========================================
  Files         428      797     +369     
  Lines      194833   277793   +82960     
  Branches    30108    53361   +23253     
==========================================
+ Hits       180907   251399   +70492     
- Misses      13599    16801    +3202     
- Partials      327     9593    +9266     

Files with missing lines	Coverage Δ
lib/events.js	99.60% <100.00%> (+6.28%)	⬆️
lib/internal/url.js	93.40% <100.00%> (+12.45%)	⬆️
lib/internal/fs/promises.js	91.13% <80.00%> (+2.07%)	⬆️
lib/internal/source_map/source_map_cache_map.js	85.95% <0.00%> (ø)
lib/internal/streams/readable.js	95.91% <88.88%> (ø)
lib/internal/vfs/watcher.js	95.21% <83.33%> (ø)
lib/internal/webstreams/readablestream.js	98.11% <85.71%> (+0.04%)	⬆️

... and 497 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[trivikr]

FYI @jasnell as the original discussion was in #63521 (comment) (also linked in PR description #63526)

[LiviaMedeiros]

The performance boost looks promising. Is this applicable to IterResult?

[bakkot]

Is this applicable to IterResult?

Assuming you mean this one, no, the objects it creates have a different prototype (to avoid Object.prototype.then pollution) and so cannot share the V8 map.

[nodejs-github-bot]

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

[LiviaMedeiros]

Assuming you mean this one, no, the objects it creates have a different prototype (to avoid Object.prototype.then pollution) and so cannot share the V8 map.

I wonder if reordering it purely for the sake of consistency would be worth the churn. Probably not (or at least not in the scope of this PR) so LGTM as is.