Skip to content

[Repo Assist] perf+fix: optimize pairwise, fix splitAt/tryTail enumerator disposal on exception#320

Merged
dsyme merged 4 commits intomainfrom
repo-assist/perf-fix-pairwise-splitat-20260425-0ce5ee841dd9747c
May 1, 2026
Merged

[Repo Assist] perf+fix: optimize pairwise, fix splitAt/tryTail enumerator disposal on exception#320
dsyme merged 4 commits intomainfrom
repo-assist/perf-fix-pairwise-splitat-20260425-0ce5ee841dd9747c

Conversation

@github-actions
Copy link
Copy Markdown
Contributor

🤖 This PR was created by Repo Assist, an automated AI assistant.

Summary

Two improvements in one PR: a performance optimization (Task 8) and a bug fix (Task 3).


Task 8 — Performance: AsyncSeq.pairwise

Root cause: The previous implementation used Option boxing for the prev element (prev <- Some v on every iteration), allocating a new heap object per step.

Fix: Replace 'T option with a hasPrev: bool flag and a direct mutable prev: 'T field.

// Before — allocates Some v on every iteration
let mutable prev = None
while b.IsSome do
    match prev with
    | None -> ()
    | Some p -> yield (p, v)
    prev <- Some v   // ← heap allocation

// After — zero allocation per step
let mutable hasPrev = false
let mutable prev = Unchecked.defaultof<'T>
while b.IsSome do
    if hasPrev then yield (prev, v)
    hasPrev <- true
    prev <- v        // ← no allocation

Benefit: Eliminates one 'T option heap allocation per input element, reducing GC pressure for long sequences.


Task 3 — Bug fix: splitAt and tryTail enumerator disposal on exception

Root cause: Both functions obtained an enumerator with let ie = source.GetEnumerator() (not use ie), intending to transfer ownership to the returned AsyncSeq. However, if the source threw an exception during the initial MoveNext() call, ie.Dispose() was never called — causing a resource leak.

Fix: Wrap the initial async body in try...with ex -> ie.Dispose(); return raise ex so the enumerator is always disposed on exception or cancellation.

Bonus: The let cur = ref b ref cell in splitAt's rest sequence was also replaced with let mutable cur = b (no allocation, same semantics).

Trade-off: The "caller discards the rest without enumerating it" case is a pre-existing limitation of this API design and is not addressed here (it would require a different return type).


Test Status

Build: 0 errors, pre-existing warnings only
Tests: 425/425 passed (3 new tests added)

  • AsyncSeq.pairwise with many elements produces correct pairs — validates the optimization produces correct output for a 10-element sequence
  • AsyncSeq.splitAt disposes enumerator when source throws during collection — verifies disposal on exception mid-collection
  • AsyncSeq.tryTail disposes enumerator when source throws on first MoveNext — verifies disposal when first async step fails

Generated by 🌈 Repo Assist, see workflow run. Learn more.

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/repo-assist.md

Generated by 🌈 Repo Assist, see workflow run. Learn more.

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/repo-assist.md@96b9d4c39aa22359c0b38265927eadb31dcf4e2a

…on exception

Task 8 (Performance): Optimize AsyncSeq.pairwise to use a hasPrev flag
and mutable field instead of wrapping prev in Some on every step.
Eliminates per-element heap allocation for long sequences.

Task 3 (Bug fix): Fix resource leak in splitAt and tryTail.
Previously, if the source sequence threw an exception during the initial
MoveNext() call, the underlying enumerator was never disposed.
Added try...with to call ie.Dispose() on exception.
Also replaces 'ref' cell with 'mutable' in splitAt's rest sequence.

Tests: 425/425 pass (3 new tests added)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
System.Threading.Channels (resolved as 10.0.7 via Version="*")
transitively requires Microsoft.Bcl.AsyncInterfaces >= 10.0.7, but
the project pinned it at 10.0.6. This produced 5 NU1605 package-
downgrade warnings on every restore/build.

Bumping the direct reference to 10.0.7 aligns both packages to the
same .NET 10.0.7 release, eliminating all 5 NU1605 warnings.
Total build warnings: 12 → 7.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown
Contributor Author

github-actions Bot commented May 1, 2026

Commit pushed: 7bfdf35

Generated by 🌈 Repo Assist, see workflow run. Learn more.

@dsyme dsyme marked this pull request as ready for review May 1, 2026 11:39
@dsyme dsyme merged commit 7e99085 into main May 1, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant