refactor(jq): thread one Reentry through the owned re-entry points instead of three booleans - #3137
Conversation
d6bf754 to
f3b47ed
Compare
CoverageTotal: 93.94% ⚪ 0.02 pp vs Comparing
🔇 0 ignored region(s), 174 tolerated region(s)
Patch coveragePatch: 98.32% (528/537 new lines covered)
Uncovered new lines (9)
Indirect coverage changes🔴 0 lines lost coverage, 🟢 15 lines gained coverage on unchanged code. Indirect changes
|
CoverageTotal: 94.04% ⚪ 0.02 pp vs Comparing
🔇 0 ignored region(s), 173 tolerated region(s)
Patch coveragePatch: 98.32% (528/537 new lines covered)
Uncovered new lines (9)
Indirect coverage changes🔴 0 lines lost coverage, 🟢 15 lines gained coverage on unchanged code. Indirect changes
|
…stead of three booleans Every owned re-entry in eval.rs (`eval_each_owned`, `eval_owned_input`, `eval_owned_expr_fork`, `eval_owned_multi_first`, `update_root_with_filter`, `each_recurse_walk`) and eval_generic.rs's `eval_on_owned` now takes a `Reentry` saying what its value's root is: `Against(RootWitness)` to reroot the expression's `Snapshot` markers against a named root at the re-entry, `Proven` when a funnel, an enclosing re-entry or a once-per-fold/loop hoist already did, `REBUILT` = `Against(Owned)` for a value no document node backs. It replaces the three booleans that encoded the same fact (`trackable` at the resolver sinks, `ambient` in `|=`'s root leaf and `until`/`while`, `resolver_free` in the folds) and the thirteen "plain vs `_bridged`" twins, so a new re-entry has to name its root rather than pick a twin (#3122). `reroot_for_reentry` is the one derivation: #3036's fused single-walk precheck (a marker the rewrite would change *and* a resolver-reaching node, else `Cow::Borrowed`) over #3037's `reroot_markers` -- #2642's demotion and, in jq mode, #3037's promotion of an untracked marker that *is* the root, one walk, one per-marker rule (`reroot_rewrite`) shared with `reroot_markers` so the precheck and the rewrite cannot disagree. `demote_for_reentry` is its demote-only twin for the hoists whose root is always `Owned`. Applying the precheck to the funnels' cursor witnesses is what took the per-record `as` body -3.6% Ir on a 7950X. What stayed, and why: `trackable` remains the resolver's register-provenance flag (it also drives path-shape decisions); `Reentry::at_register` is its one conversion. `ambient` remains `LoopOperand::fork`'s round selector between an operand and its pre-demoted twin, both `Proven`. `resolver_free` and `fold_update_is_resolver_free` are gone: `reduce_forks`/`foreach_forks` demote UPDATE/EXTRACT once per fold ahead of the loop-variable substitution (`substitute_foreach_steps` substitutes literals, never a marker) and pass `Proven` per element. `each_recurse_generic` names the cursor's witness for `f`'s level-0 run; `eval_owned_input_bridge` (no fast path, no rewrite) exists for the fast-path agreement tests. `#[inline(always)]` on the two thin wrappers and `Reentry::reroot` keeps a `Proven` re-entry free per step (`until` was +1.4% Ir through them before, +0.0007% after; the release binary is 25 KB smaller than main's). Rebased onto #3037 (`reroot_markers`) and #2872 (the backtracking pattern matcher) as one commit: the matcher's computed-key twin and the folds' `resolver_free` plumbing on main took the same conversion. Pinned by `demote_for_reentry_rebuilds_only_a_resolver_reaching_marker_3122` (both directions of the rewrite, both modes, all three `Reentry` forms) and `test_tracked_var_recurse_level_zero_names_its_root_3122` (captured live against jq 1.7.1). Closes #3122
Adds a real test for each new/changed line the coverage bot flagged uncovered (eval_owned_with_file_index's fast path, the AsPattern-hidden pipe demotion door, the input-queue bridge's cursor-preserving twin, the cursor-less pipe's owned fallback, and the non-navigable path/non-identity reindex fallback), plus a unit test proving rewrite_markers still walks past an already-matching sibling marker to reach an unrelated one -- the indirect regression the bot flagged in unchanged code, now confirmed reachable rather than dead. The remaining flagged lines are pre-existing, by-design-unreachable panic arms whose own test intent didn't change; those get `omni-dev: coverage tolerate` annotations instead.
e80bee1 to
2ec3921
Compare
Closes #3122.
What changed
eval.rs's owned re-entries -- every call that serializes anOwnedValueinto a throwawaydocument and evaluates an expression over it -- decided whether the
Snapshotmarkers in thatexpression could still name the document's root through three independently named booleans
(
trackableat the resolver sinks,ambientin|='s root leaf anduntil/while,resolver_freein the folds) and thirteen "plain vs_bridged" twin functions. A new re-entryhad to guess which twin to call, with nothing forcing it to say what its value's root was -- the
class of gap behind #2642 and #3036.
Every re-entry (
eval_each_owned,eval_owned_input,eval_owned_expr_fork,eval_owned_multi_first,update_root_with_filter,each_recurse_walk, andeval_generic.rs'seval_on_owned) now takes oneReentry:Against(RootWitness): reroot against a named root at the re-entry -- jq: a root-bound$xmatches a rebuilt equal-valued copy — TrackedVar's value witness writes where jq refuses #2642's demotion and,in jq mode, jq: a $var bound from a navigated position refuses at that same node (.a as $y | .a | ($y.b) = 9) #3037's promotion of an untracked marker that is the root, one walk. The generic
funnels pass the witness they already compute and stop pre-rewriting.
Proven: the markers were already demoted for this value's document upstream -- a funnel,an enclosing re-entry, or a static operand the caller hoisted through
demote_for_reentryonce per fold/loop.
REBUILT=Against(Owned): no document node backs the value.reroot_for_reentryis the one derivation -- #3036's fused single-walk precheck (a marker therewrite would change and a resolver-entering node, else
Cow::Borrowed) over #3037'sreroot_markers, sharing its per-marker rule (reroot_rewrite) so the two cannot disagree --so the "no resolver node, no rebuild" gate now covers every funnel too.
demote_for_reentryisits demote-only twin for the hoists whose root is always
Owned. The twoPatharms' identityroute rewrites the resolver's argument alone, so they keep the unconditional
reroot_markers(the resolver-reaching node is the builtin around the argument).
Rebased onto #3037 and #2872 as a single commit (review): #3037 replaced the funnels'
demotion with
reroot_markersand #2872 restructured the folds and the destructuring matcherunder this change, so the four original commits were re-applied as one; the matcher's
computed-key twin and the folds'
resolver_freeplumbing onmaintook the same conversion.Reentry::Againstat a funnel therefore carries #3037's promotion:.a as $y | .a | path($y)answers
[]as onmain, pinned by #3037's own tests.What stayed, and why:
trackableremains a resolver-internal register-provenance flag: it also drives path-shapedecisions (
Snapshot::Noforcing, register identity,is_primitive).Reentry::at_registeris its one conversion at the eleven sinks. The resolver holds no cursor and no
RootWitness(
Frameis an invocation id and a path), soProvenis the honest encoding of "the funnelthat built this document already demoted for it, and the register is a navigated node of
it", not a witness the resolver could mint.
ambientremainsLoopOperand::fork's round selector between the operand and itspre-demoted twin (both
Proven); it is loop state, not a re-entry parameter.resolver_freeandfold_update_is_resolver_freeare gone:reduce_forks/foreach_forksrun the demotion once per fold, ahead of the loop-variable substitution (which never mints a
marker or a resolver-entering node), and pass
Provenper element -- the once-per-fold costjq: #2642 fabrication still live when the bind and the rebuild both run inside eval.rs (input builtins, |= RHS, fold UPDATE) #3036's review measured, now by construction.
is_root_pathstays: it is a path-shape predicate, not a witness computation.try_payload_root,RootWitness::of,OwnedIdentity::root_witnessremain the three witnesssources; they answer three different questions.
A suspected hole the design review raised --
each_recurse_generichandingfto the walkwith no cursor witness -- does not reproduce on
main(the owned-identity pipe demotes thestage first); the site now names its witness anyway and the shape is pinned in
test_tracked_var_recurse_level_zero_names_its_root_3122.Verification
63ab6c2d5:cargo test --features cliall passed (0 failed),cargo test --no-default-features,cargo doc --no-deps --all-featureswithRUSTDOCFLAGS=-D warnings, both clippy legs,cargo fmt --check: clean.scripts/jq-bind-origin-oracle-sweep.shagainst/usr/bin/jq1.7.1 (post-rebase, withjq: a $var bound from a navigated position refuses at that same node (.a as $y | .a | ($y.b) = 9) #3037's rows): agree=250 fabricate=0 mismatch=0 refuse-only=40 refuse-only-NEW=0.
scripts/jq-bind-origin-fuzz.py, 3 seeds x 2000 programs: 0 fabricate, 0 mismatch on every seed (agree 1790/1787/1767, refuse-only 204/209/225). (Pre-rebase, with--baseline= the merge-base binary982f7c4b9: 0 fabricate, 0 mismatch on every seed.)foreach, until, repeat, try/catch, paths, with_entries, any): every row answers as
maindoes, and the
.a as $y | .a | ...rows answer as jq does through jq: a $var bound from a navigated position refuses at that same node (.a as $y | .a | ($y.b) = 9) #3037's promotion; theremaining refuse-only rows are the fold loop-variable, owned-root and positional classes
limitations.mdalready records.demote_for_reentry_rebuilds_only_a_resolver_reaching_marker_3122(unit) andtest_tracked_var_recurse_level_zero_names_its_root_3122(CLI, captured live against jq 1.7.1).Perf A/B (both architectures)
scripts/ab-cli.py --tool jq --reps 7, interleaved, output identity + rc gated, 2.25 MBusersdocument (22,528 records) and its NDJSON twin for the-n inputsshape. Before =982f7c4b9(the merge-base), after = this branch. The control run (before vs a copy ofitself) is each box's noise floor.
Apple M4 Pro (
johns-mac-mini), final binaryControl floor: median of medians −0.46%, range −2.3%..+3.2%, 3/10 rows slower.
reduce .users[] as $r (0; . + $r.score)[.users[] | . as $x | {s:.score} | $x.id]. as $x | .users[] |= (.score += 1).users | length | . * 4 | until(. < 0; . - 1)[paths(select(type == "number"))]del(.users[] | select(.score < 50)).users[0] as $x | [path(.users[] | select(. == $x))].users | length(control).(control)-n '[inputs | . as $x | {s:.score} | $x.id]'Median of medians +0.25%, mean −0.60%, range −3.5%..+1.4%, 5/9 rows slower: inside the floor.
(The fold-write row
reduce .users[0:2000][] ..was timed on the pre-inline binary only:+1.8% med / −0.1% min on the M4 Pro, +2.8%/+0.6% then +1.7%/+0.6% on the 7950X, against a
control that itself read +3.2% med on the M4 Pro -- a 0.9 s row dominated by the pre-existing
quadratic below.)
AMD Ryzen 9 7950X (
terminus,taskset -c 0), wall-clock and cachegrind, pre-inline binaryControl floor: median of medians −0.31%, range −2.0%..+0.9%, 3/10 rows slower. Two independent
main passes agreed (+2.15% / +1.96% median, 8/10 and 7/9 rows slower), so each moved row was
attributed with
valgrind --tool=cachegrindinstruction counts (deterministic, no timing noise):reduce .users[] as $r (0; . + $r.score)[.users[] | . as $x | {s:.score} | $x.id]. as $x | .users[] |= (.score += 1).users | length | . * 4 | until(. < 0; . - 1)[paths(select(type == "number"))]del(.users[] | select(.score < 50)).users[0] as $x | [path(.users[] | select(. == $x))].users | length(control).(control)-n '[inputs | . as $x | {s:.score} | $x.id]'Every wall-clock move except two sits on a flat instruction count -- the +10.6% on a row that
cannot reach this change is the x86 code-layout bias #2603/#2655 record (the binary shrank
40 KB). The two real instruction deltas: the per-record
asbody is −3.6% Ir (the funnel's"no resolver node, no rebuild" gate now applies to the identity pipe's stage demotion), and
untilwas +1.4% Ir (~43 instructions per step) from the 32-byteReentrytravelling by valuethrough two thin wrappers per step. A plain
#[inline]hint changed nothing under the pinnedfat-LTO profile (
head2counts identical tohead);#[inline(always)]oneval_owned_expr_fork,eval_owned_inputandReentry::demote(the last commit) brought thatrow to +0.0007% Ir with every other row unchanged (
head3). The release binary is 25 KB smaller thanmain's(10,305,472 vs 10,330,976 bytes) with the
inline(always)in place:.users | lengthuntilloop, 90k steps. as $x | .users[] |= (.score += 1)del(.users[] | select(.score < 50))[paths(select(type == "number"))][.users[] | . as $x | {s:.score} | $x.id]reduce .users[] as $r (0; . + $r.score)Final wall-clock pass with that binary on the 7950X: median of medians +0.82%, mean −0.19%,
range −4.6%..+1.5% (7/9 rows slower, each inside or at the control floor); input-queue shape
−0.6% (control −0.1%).
One shape was scaled down for the A/B:
reduce .users[] as $r ({}; .[$r.name] = $r.score)takes 110 s on
mainat 22,528 elements (0.9 s at 2,000, 3.4 s at 4,000 -- quadratic throughthe resolver, unrelated to this branch; #2086/#2152/#2157 cover the bare-accumulator shapes,
not this assignment one). Timed at 2,000 elements here; follow-up issue text is ready if wanted.