Skip to content

perf(dump): refactor async iterator merging with promise slots#33

Open
MrOrz wants to merge 2 commits intomasterfrom
refactor/merge-async-iterators
Open

perf(dump): refactor async iterator merging with promise slots#33
MrOrz wants to merge 2 commits intomasterfrom
refactor/merge-async-iterators

Conversation

@MrOrz
Copy link
Copy Markdown
Member

@MrOrz MrOrz commented Apr 23, 2026

Refactor the async iterator merging logic to use a more efficient promise slot pattern with NEVER_RESOLVE and index-based tracking. This improves performance by avoiding O(N) searches and frequent array allocations.

Replace the index-based findIndex/splice approach in mergeAsyncIterables with a more efficient "promise slot" pattern.

By maintaining a fixed-length array of promises and using Promise.race, we:
- Achieve O(1) slot updates after each resolve.
- Avoid unnecessary array allocations and filter/splice calls in the loop.
- Use NEVER_RESOLVE to cleanly manage finished generators.

Also rename the function to mergeAsyncGenerators to better reflect its specialized usage with async generators.

Co-authored-by: Gemini CLI <gemini-cli@google.com>
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request replaces the mergeAsyncIterables function with mergeAsyncGenerators, implementing a promise slot pattern to merge multiple async generators. The review feedback identifies opportunities to improve throughput by refilling promise slots before yielding and to prevent resource leaks by ensuring source generators are properly closed via a try...finally block.

Comment thread dumpOpenData.js Outdated
Comment thread dumpOpenData.js
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Base automatically changed from refactor/migrate-es9-node24-9232966085038031085 to master April 24, 2026 04:01
@MrOrz
Copy link
Copy Markdown
Member Author

MrOrz commented Apr 24, 2026

@MrOrz
Copy link
Copy Markdown
Member Author

MrOrz commented Apr 24, 2026

@claude do you have any idea why it keeps OOM when scanning the index and output?
The original loop does not OOM, but my implementation OOMs.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 24, 2026

Claude encountered an error —— View job


I'll analyze this and get back to you.

@nonumpa
Copy link
Copy Markdown
Member

nonumpa commented Apr 24, 2026

The likely cause of the OOM is the NEVER_RESOLVE sentinel in mergeAsyncGenerators().

In the previous implementation, finished iterators were removed from the Promise.race() set with entries.splice(...).

In the new implementation, a finished slot is replaced with a promise that never settles:

const NEVER_RESOLVE = new Promise(() => {});
promises[winner.idx] = NEVER_RESOLVE;

while the loop continues to run:

await Promise.race(promises);

That means the finished slot stays in the race forever. If Promise.race(promises) keeps attaching reactions to every still-pending promise, those reactions can accumulate on the same never-settling promise over the full scan/output run, which would explain the memory growth and the OOM.

The old loop did not have this behavior because finished iterators were removed entirely instead of being kept as permanently pending slots.

@MrOrz
Copy link
Copy Markdown
Member Author

MrOrz commented Apr 26, 2026

But NEVER_RESOLVE gets only instantiated once, all loop executions share exactly the same NEVER_RESOLVE instance. It should only occupy 1 slot of memory, the others are just references to the same promise.

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.

2 participants