Skip to content

perf(builtin): count rev_foldi's index up instead of deriving it per iteration - #4117

Merged
bobzhang merged 1 commit into
mainfrom
hongbo/rev-foldi-count-up
Aug 20, 2026
Merged

perf(builtin): count rev_foldi's index up instead of deriving it per iteration#4117
bobzhang merged 1 commit into
mainfrom
hongbo/rev-foldi-count-up

Conversation

@bobzhang

Copy link
Copy Markdown
Contributor

Summary

Completes #3784 (thanks @mizchi) on current main. Three of that PR's four FixedArray reverse-iteration optimizations have already landed; the remaining delta is rev_foldi, which still derives its ascending index as len - i - 1 on every iteration of the descending range loop. That PR proposed a C-style three-variable loop; this PR takes the same optimization while keeping the range form the neighboring functions already use — the index simply becomes a second loop state:

pub fn[A, B] FixedArray::rev_foldi(
  self : FixedArray[A],
  init~ : B,
  f : (Int, B, A) -> B raise?,
) -> B raise? {
  for i in self.length()>..0; index = 0, acc = init {
    continue index + 1, f(index, acc, self.unsafe_get(i))
  } nobreak {
    acc
  }
}

Benchmarks (fresh, against current main — #3784's table predates the three already-merged changes)

rev_foldi, n=100K sum fold, moon bench --release:

backend main this PR
native 14.88 µs 11.19 µs 1.33x
js 72.4 µs 59.9 µs 1.21x
wasm-gc 75.5 µs 65.6 µs 1.15x

The readable range form measures identical to #3784's C-style loop (11.19 vs 11.20 µs native) — the C-style rewrite buys nothing over for i in … >..0; index = 0, acc = init.

Why it's faster: per-element times (~0.1 ns) show the fold is fully inlined and auto-vectorized in this benchmark; len - i - 1 adds a per-lane subtract dependency that vectorizes worse than an independently counting index. With an opaque, non-inlinable closure the difference shrinks accordingly.

Tests

Adopts #3784's regression test pinning the reversed index/element pairing ([10,20,30] traced to 30120210, i.e. pairs (0,30),(1,20),(2,10)), which a miscounted index would break.

Review

Codex CLI (xhigh), first-round sign-off: index/element pairing verified against the documented reverse-range and simultaneous-update semantics (f receives the pre-increment index), the trace recomputed by hand, raise-polymorphism and nobreak return intact, benchmark ratios recomputed, claims microbenchmark-scoped. No .mbti change.

Signed-off-by: Codex CLI codex@openai.com

Validation

  • builtin 2944/2944 (includes the pairing test); moon check --warn-list +unnecessary_annotation clean; moon fmt applied; moon info unchanged

🤖 Generated with Claude Code

…iteration

Completes PR #3784 (mizchi) on current main, where three of its four
FixedArray reverse-iteration optimizations have already landed. The
remaining delta is rev_foldi, which computed the ascending index as
len - i - 1 on every iteration inside the descending range loop.

Instead of that PR's C-style three-variable loop, the index becomes a
second loop state of the same readable range form:

  for i in self.length()>..0; index = 0, acc = init {
    continue index + 1, f(index, acc, self.unsafe_get(i))
  }

Measured identical to the C-style loop (11.19 vs 11.20 us native) and
faster than main on all three backends (rev_foldi, n=100K, sum fold):
native 14.88 -> 11.19us (1.33x), js 72.4 -> 59.9us (1.21x), wasm-gc
75.5 -> 65.6us (1.15x). Per-element times (~0.1ns) show the fold is
fully inlined and auto-vectorized in this benchmark; the win is the
per-lane subtract dependency of len - i - 1 vectorizing worse than a
counting index. With an opaque, non-inlinable closure the difference
shrinks accordingly.

Also adopts #3784's test pinning the reversed index/element pairing,
which a miscounted index would break.

Codex CLI review (xhigh, first-round sign-off): (index, element)
pairing verified as (0,len-1)..(len-1,0) against the documented
reverse-range and simultaneous-update semantics (f receives the
pre-increment index); the pairing-test trace recomputed to 30120210;
raise-polymorphism and the nobreak return intact; benchmark ratios
recomputed (1.33x/1.21x/1.15x) and microbenchmark-scoped.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex CLI <codex@openai.com>
Copilot AI lite review requested due to automatic review settings August 20, 2026 07:45
@bobzhang

Copy link
Copy Markdown
Contributor Author

Codex CLIreview (xhigh) — approved, first round

No blocking findings; approved.

  • rev_foldi preserves (index, element) as (0,len−1)…(len−1,0). MoonBit's reverse-range and simultaneous-update semantics confirm f receives the pre-increment index.
  • The regression trace correctly evaluates to 30120210.
  • Raise-polymorphism and nobreak accumulator return remain intact.
  • Benchmark ratios recompute to 1.3298×, 1.2087×, and 1.1509×; claims are appropriately microbenchmark-scoped.
  • No .mbti change.

Signed-off-by: Codex CLI codex@openai.com

Posted on behalf of Codex CLI by Claude Code. Validation outside its read-only sandbox: builtin 2944/2944; benchmarks on native/js/wasm-gc as tabled; the readable range form measured identical to #3784's C-style loop.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Optimizes FixedArray::rev_foldi in builtin by avoiding per-iteration derivation of the ascending index (len - i - 1) during reverse iteration, instead maintaining an explicit index loop state while keeping the existing range-style reverse loop form used nearby.

Changes:

  • Rewrite FixedArray::rev_foldi to carry index as a second loop state (index = 0) and increment it each iteration.
  • Add a regression test ensuring reversed iteration preserves correct (index, element) pairing.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@coveralls

Copy link
Copy Markdown
Collaborator

Coverage Report for CI Build 6229

Coverage remained the same at 90.734%

Details

  • Coverage remained the same as the base build.
  • Patch coverage: 2 of 2 lines across 1 file are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 17969
Covered Lines: 16304
Line Coverage: 90.73%
Coverage Strength: 332614.98 hits per line

💛 - Coveralls

@bobzhang
bobzhang merged commit 3b2e6cb into main Aug 20, 2026
17 checks passed
@bobzhang
bobzhang deleted the hongbo/rev-foldi-count-up branch August 20, 2026 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants