perf(iterator): Optimize Iter eager consumers#3642
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates several eager Iter consumers (each, any, all, eachi, fold, count, find_first, to_array, last, contains, nth, maximum, minimum) to drive the underlying iterator function (self.f) directly and decrement self.size_hint per element, so callers observing size_hint from inside a callback (or after a partial/full consumption) see an accurate remaining count.
Changes:
- Replaced
while self.next() is Some(x) { ... }patterns with explicitfor next = next_fn()loops that decrementsize_hinton each element and zero it on exhaustion. - Rewrote
maximum/minimum/last/count/nthto fit the new tracking pattern (no longer reusingfold). - Added size-hint tracking tests for eager consumers, plus benchmarks for
IteroverFixedArrayandArrayView.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| builtin/iterator.mbt | Inlines iteration loops to update size_hint per element across all eager consumers. |
| builtin/iter_test.mbt | Adds a test verifying size_hint decreases during/after eager consumption. |
| builtin/array_locals_bench_test.mbt | Adds Iter fold/each/to_array benchmarks for FixedArray and ArrayView. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if self.size_hint is Some(n) { | ||
| self.size_hint = if n > 0 { Some(n - 1) } else { Some(0) } | ||
| } |
| let mut count = 0 | ||
| let next_fn = self.f | ||
| for next = next_fn() { | ||
| match next { | ||
| Some(_) => { | ||
| if self.size_hint is Some(n) { | ||
| self.size_hint = if n > 0 { Some(n - 1) } else { Some(0) } | ||
| } | ||
| count += 1 | ||
| continue next_fn() | ||
| } | ||
| None => { | ||
| self.size_hint = Some(0) | ||
| break | ||
| } | ||
| } | ||
| } | ||
| count |
| let next_fn = self.f | ||
| for next = next_fn() { |
| let mut res = None | ||
| let next_fn = self.f | ||
| for next = next_fn() { | ||
| match next { | ||
| Some(x) => { | ||
| if self.size_hint is Some(n) { | ||
| self.size_hint = if n > 0 { Some(n - 1) } else { Some(0) } | ||
| } | ||
| match res { | ||
| Some(current) if x <= current => () | ||
| _ => res = Some(x) | ||
| } |
| match next { | ||
| Some(x) => { | ||
| if self.size_hint is Some(n) { | ||
| self.size_hint = if n > 0 { Some(n - 1) } else { Some(0) } |
mizchi
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87b1e0bb95
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Some(current) if x <= current => () | ||
| _ => res = Some(x) |
There was a problem hiding this comment.
For Double/Float iterators containing NaN, this is not equivalent to the previous if x > res logic because both NaN <= current and NaN > current are false. For example, [1.0, NaN].iter().maximum() used to keep 1.0, but this branch now falls through to res = Some(x) and returns NaN; [NaN, 1.0] changes in the opposite direction. Please preserve the old x > current update condition for partial orders.
Useful? React with 👍 / 👎.
| Some(current) if x >= current => () | ||
| _ => res = Some(x) |
There was a problem hiding this comment.
For Double/Float iterators containing NaN, this is not equivalent to the previous if x < res logic because both NaN >= current and NaN < current are false. For example, [1.0, NaN].iter().minimum() used to keep 1.0, but this branch now falls through to res = Some(x) and returns NaN; [NaN, 1.0] changes in the opposite direction. Please preserve the old x < current update condition for partial orders.
Useful? React with 👍 / 👎.
mizchi
force-pushed
the
codex/optimize-iter-eager-consumers
branch
from
87b1e0b to
ab09ccc
Compare
mizchi
force-pushed
the
codex/optimize-iter-eager-consumers
branch
from
ab09ccc to
3e91847
Compare
|
Thanks for revealing this optimization opportunity. We are working on improving the inlining so I think it's the compiler's duty for handle the situation here. |
Summary
This PR optimizes eager
Iterconsumers by avoiding the extraself.next()call inside hot loops.Consumers such as
each,fold,to_array,find_first,contains,nth,maximum, andminimumnow call the iterator's internalself.fdirectly and use the same#inlinesize-hint bookkeeping helpers asnext. This keeps the observablesize_hintbehavior while reducing call overhead, especially foriter().foldon wasm-gc.Changes
Iterconsumers to callself.fdirectly in hot loopssize_hintbookkeeping in private#inlinehelpers shared withnextsize_hintvalues asNoneafter underflow until exhaustionmaximum/minimumNaN behavior by keeping the oldx > res/x < resupdate conditionscountdelegated tofoldsize_hintbehavior during eager consumersFixedArray/ArrayViewiterator benchmarks forfold,each, andto_arrayBenchmark
wasm-gc --release,n=100000FixedArray::iter foldFixedArray::iter eachFixedArray::iter to_arrayArrayView::iter foldArrayView::iter eachArrayView::iter to_arrayVector::iter foldVector::iter eachVector::iter to_arrayI also checked RSS with
hyperfine+/usr/bin/time -l. The measurement covers the wholemoon benchprocess rather than an individual MoonBit function; median RSS was about 1.2-1.6 MiB higher than baseline, with no large regression observed.Validation
moon test: 6510 passed.Review notes
The eager consumers intentionally bypass
nextto avoid the extra method call in hot loops. To keep the code paths aligned,nextand the eager consumers now share the same private#inlinehelpers forsize_hintupdates.