Repository navigation
Make stage execution typed end to end - #47
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: af1d8bc43e
ℹ️ 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".
|
|
||
| // Computed once per append rather than per read: `names`, `size` and `count` are all called while a | ||
| // pipeline is being built, and the next stage's name is derived from `size`. | ||
| private[exec] val entries: List[Entry] = init.entries :+ entry |
There was a problem hiding this comment.
When stages are generated dynamically, every Link retains its predecessor while also copying all of init.entries via :+; a chain of n stages therefore permanently retains O(n²) list nodes. Large generated pipelines can consume excessive heap or fail during construction, so store entries in a structurally shared order (for example, prepend and reverse only when metadata is requested).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5e38625. Entries are now held most-recent-first, so a link conses onto the preceding spine instead of copying it: appending is constant time and the whole chain holds one list. size is a per-link val because a builder reads it on every append to name the next stage. names is the only caller that needs running order, and it reverses once per build.
| private[exec] def arrow( | ||
| decorate: StageDecorator[F], | ||
| )(implicit F: EffectSystem[F], | ||
| ): Kleisli[F, In, Out] = init.arrow(decorate).andThen(decorate(entry.name, stage)) |
There was a problem hiding this comment.
When a pipeline contains thousands of programmatically generated stages, arrow recursively descends through one JVM stack frame per Link before creating the effect. The previous list-based implementation used foldLeft, so sufficiently deep pipelines now throw StackOverflowError before any stage executes; construct the composed arrow with a stack-safe traversal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Measured rather than assumed: 10k stages build and run in 12ms, 50k overflow. So the bound is real, and it is being accepted rather than fixed.
Folding left needs the chain reversed, and reversing a type-aligned sequence needs the existential cast this type exists to remove, so a stack-safe build would reintroduce exactly what the PR removes. The bound is now documented on arrow and StageChainSpec pins 10k, which also guards the entries fix. For context, every stage carries a span and a START/COMPLETE pair, so a chain anywhere near the bound is unusable for reasons unrelated to the stack.
There was a problem hiding this comment.
Correcting my earlier reply on this thread: the numbers I gave were measured on macOS, whose main thread gets 8MB of stack. CI proved them wrong. The 10k test passed on macos-latest and overflowed on ubuntu and windows (~1MB stacks), and on a 512k stack the eager walk overflowed between 2k and 4k. So the bound was not "far past any pipeline" and it was not a property of the chain at all, it was a property of whichever thread happened to run it.
Fixed rather than documented, in 07e1242. Each Link.arrow now defers the rest of the chain through EffectSystem.suspend instead of building it with andThen, so the descent happens inside F s own loop. No casts added. The test now runs 50k stages on a thread with an explicit 512k stack, which the old eager version could not survive, so the property is pinned to a named stack size rather than to the runner.
Two costs, both stated in the scaladoc: the links are walked once per run rather than once per build, and stack safety now rests on F.flatMap and F.suspend being stack-safe, which MonadError does not promise. Thanks for pushing on this one.
Stages are now held in a StageChain that carries the type between each pair of stages, so StageComposer builds the pipeline arrow without the two Kleisli[F, Any, Any] casts.
Stage counts in metadata now come from the kind a builder gives each stage instead of five different per-builder checks.