fix(tiers): evaluate :or destructuring defaults eagerly in every tier - #367
Merged
Conversation
Clojure's `destructure` expands a symbol carrying an `:or` entry to
`(get m :k default)`. `get` is an ordinary function, so the default is
evaluated whether or not the key is present. The tree-walker does that;
the compiled tiers did not, and the two disagreed:
- `lower_with_default` lowered the default into the `then` arm of an
`IsNil` branch, so it only ran on a miss. Lower it into the current
block instead — straight-line and unconditional — and keep the branch
purely as a select over two already-computed values.
- `execute_ir` bound the arity's destructuring patterns into the env via
`bind_fn_params` on top of the prologue the lowerer emits, so with the
eager lowering above each default would run twice per call. The ANF
lowerer never emits `LoadLocal`, so those env bindings are never read:
bind only the named params there (`bind_fn_params_positional`), and let
the prologue own the destructuring.
Because tier-up happens partway through a run, and lowering is driven by a
background worker, the old split made a side-effecting or throwing default
fire a nondeterministic number of times: the issue's reproduction reported
133/130/140/130/144 evaluations out of 200 calls across five runs of the
same binary. It now reports 200 in every tier and every run.
Pinned in two places: `execution_tier_parity` gains an `:or` case with a
side-effect counter, so tree-walk, eager IR, JIT and AOT must agree on the
count (it fails without either half of this fix), and a new
`destructure_or_default_eager` test runs the discriminating programs
through the tree-walker and the IR tier side by side.
Fixes #363
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLQqFN41sWfs1hF5uyfN8N
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Clojure's
destructureexpands a symbol carrying an:orentry to(get m :k default).getis an ordinary function, so the default isevaluated whether or not the key is present. The tree-walker does that;
the compiled tiers did not, and the two disagreed:
lower_with_defaultlowered the default into thethenarm of anIsNilbranch, so it only ran on a miss. Lower it into the currentblock instead — straight-line and unconditional — and keep the branch
purely as a select over two already-computed values.
execute_irbound the arity's destructuring patterns into the env viabind_fn_paramson top of the prologue the lowerer emits, so with theeager lowering above each default would run twice per call. The ANF
lowerer never emits
LoadLocal, so those env bindings are never read:bind only the named params there (
bind_fn_params_positional), and letthe prologue own the destructuring.
Because tier-up happens partway through a run, and lowering is driven by a
background worker, the old split made a side-effecting or throwing default
fire a nondeterministic number of times: the issue's reproduction reported
133/130/140/130/144 evaluations out of 200 calls across five runs of the
same binary. It now reports 200 in every tier and every run.
Pinned in two places:
execution_tier_paritygains an:orcase with aside-effect counter, so tree-walk, eager IR, JIT and AOT must agree on the
count (it fails without either half of this fix), and a new
destructure_or_default_eagertest runs the discriminating programsthrough the tree-walker and the IR tier side by side.
Fixes #363
Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01NLQqFN41sWfs1hF5uyfN8N