fix(jq): link each dependency module once and forward to it through stubs (#2955, #3058) - #3150
Conversation
CoverageTotal: 93.96% ⚪ 0.01 pp vs Comparing
🔇 0 ignored region(s), 175 tolerated region(s)
Patch coveragePatch: 98.54% (338/343 new lines covered)
Uncovered new lines (5)
Indirect coverage changes🔴 1 lines lost coverage, 🟢 0 lines gained coverage on unchanged code. Indirect changes
|
CoverageTotal: 94.05% ⚪ 0.01 pp vs Comparing
🔇 0 ignored region(s), 174 tolerated region(s)
Patch coveragePatch: 98.54% (338/343 new lines covered)
Uncovered new lines (5)
Indirect coverage changes🔴 1 lines lost coverage, 🟢 0 lines gained coverage on unchanged code. Indirect changes
|
873618f to
56b3803
Compare
|
Rebased onto |
…tubs (#2955, #3058) Binding a module's dependencies by copying their bodies into every def that reached them compounded down a chain: each level's bodies already carried the level below, so a chain whose defs each call F defs of the level below held F^L copies of the bottom one. The issue's 14-level fan-out-2 chain peaked at 681 MB against jq's 2 MB. A module some other module depends on is now emitted once, outermost in `process_program`, as an ordinary run whose begin marker carries a hidden alias (`ModuleRun::link_alias`) and whose defs are named `ModuleRun::link_name` (`<NUL>link:<id>::<name>`). Inside the run a bare sibling call misses those names, floors, and is retried under the alias by the code #2989 added for imported modules, so the evaluator binds it unchanged. Each consuming def's body is wrapped in one forwarding stub per dependency (name, arity) it calls directly (`dep_stubs_for`), with the two exclusions the copying loader had; the transitive closure and #2962's rename (`rename_dep_calls`, `renamed_dep`) are gone, since a dependency's free names resolve in its own run. The one resolver change: `scan_scope` steps over an open begin marker when the name looked up is a link name, so a stub inside any run reaches the run outside. Link runs are wrapped in dependency post-order with each module's directives visited last-declared first (`hoist_order`), which is the order jq 1.7.1 reports cross-module compile errors in; and a dependency body now exists once, so an error in it is reported once (#3058). Only defs reached by name from the main filter -- through the top-level runs, retrying an imported module's bare sibling calls under its alias, then the linked modules dependents-first -- are linked, which keeps a wide chain the filter uses one def of at one def per level. Measured (release, this machine, output identical to jq): 8x6x3 142 MB -> 34 MB, 12x4x2 162 MB -> 36 MB, 14x4x2 681 MB -> 112 MB and 0.38 s -> 0.03 s; the recursion ceiling through a linked dependency is unchanged at 19494. The processed program is now linear in chain depth (169 / 241 / 313 nodes at 6 / 8 / 10 levels, against 1253 / 5093 / 20453).
…d a chain size guard (#2955) - `link_size_guard_2955` (jq_runner.rs): counts the processed program's nodes for the issue's fan-out-2 chain at 6 / 8 / 10 levels and asserts a constant per-level delta; it failed against the copying loader (1253 / 5093 / 20453) and cannot pass by accident. A sibling test pins that `unqualified_def_names` never returns a link spelling. - `test_fan_out_module_chain_stays_linear_2955`: the issue's 8x6x3 and 12x4x2 shapes end to end, with jq's answers. - `test_linked_dependency_keeps_jq_scoping_2955`: ten rows captured from jq 1.7.1 -- a nested import's bare sibling call, an imported module's sibling reached bare that has its own dependency (the referenced closure must retry the bare call under the alias; found by review), one dependency under two includers, `$param` and closure params and `path()` through a stub, a parameter named like its def, two same-name defs, siblings plus a deeper dependency, recursion inside and through a dependency. - `test_dependency_errors_report_in_jq_order_2955`: a dependency's errors before its includer's, last-declared first, deepest first. - `test_dependency_error_is_reported_once_3058` replaces the pin of the once-per-copy count with jq's single report, for a variable, a call, and the include-plus-dependency shape. - `test_depth_guard_names_a_linked_def_as_written_2955`. - The `_2962` matrix's doc comment no longer describes a rename; its 30 rows are unchanged.
#3058 residual ADR-0023 gains an "Amendment (#2955, implemented)" section -- one link run per dependency module with a hidden alias, forwarding stubs, the floor-crossing lookup, jq's error order from placement, the referenced closure, and the soundness argument re-derived from decision 5 -- and marks the #2962 rename superseded and the "symbolic binding" alternative taken. `Expr::Shared` splicing is recorded as rejected. limitations.md closes "Deeply chained modules compound in memory" with the before/after table, closes #3058 with its include-plus-dependency residual, rewrites the #2962 section's mechanism, fixes its row count to 30, and records what remains: the evaluator's own per-call bound-body retention (the same 56 defs in one file cost 58 MB, `fib(20)` 234 MB, `fib(21)` overflows the stack) and the wide-chain startup cost of a long top-level chain (a 20x50 chain with every top-level def used: 31 ms -> 137 ms on an M4 Pro; one def used: 21 ms -> 9 ms). jq-language.md's module paragraph describes the new binding.
…dress #2955 review findings - `dep_stubs_for` compared a dependency's *bare* name against the consuming def's own (name, arity) and parameter names even for an `import ... as ns` group, whose stub is spelt `ns::name` and cannot clash with either. The stub was dropped and a working call became a spurious compile error. Rows R34-R36 in the #2962 matrix pin the three shapes (own name, a parameter, the def's own recursive call), captured from jq 1.7.1. - `hoist_order` takes the top-level directives' already-interned run ids from `process_program` instead of re-resolving every path through the filesystem. - The per-module referenced closure indexes siblings by name once instead of scanning the module per call. - `report_unresolved_call` and the bare-form fallback map a def name through `ModuleRun::display_name`, so a link spelling can never reach stderr. - Doc comments on `link_keys` (why it is keyed by literal path) and on the two twin fixed points.
…ads by canonical path hoist_order relied on deps_of's insertion order, but module_dep_defs records a module's includes before its imports regardless of interleaving, reversing jq's compile-error order when a dependency module mixes both directives. Sorting deps_of entries by their own decl_index, the same way top_ids already is, restores it (#2955 review). Also key loaded_modules by the canonical file (ModuleLoader::run_key) instead of the literal spelling a caller wrote, so two spellings of one module share one load instead of parsing and binding it twice -- this also makes link_keys redundant with run_origins, so it's retired. Finally, factor the two reachability fixed-point loops in link_dependency_runs onto one shared grow_to_fixed_point helper, so a future fix to the iteration itself doesn't need to be applied twice.
56b3803 to
0bcfcb4
Compare
Closes #2955. Closes #3058.
What
Binding a module's
include/importdependencies by copying their bodies into every def that reached them compounded down a module chain (F^Lcopies of the bottom level). A dependency module is now emitted once, outermost inprocess_program, as an ordinary run whose begin marker carries a hidden alias and whose defs are named<NUL>link:<id>::<name>; each consuming def's body is wrapped in one forwarding stub per dependency (name, arity) it calls directly. Inside the run a bare sibling call is retried under the alias by the code #2989 added for imported modules, so the evaluator is untouched. The one resolver change is thatscan_scopesteps over an open floor for a link-name lookup.#2962's rename machinery and the transitive closure are gone. ADR-0023 gains the amendment with the soundness argument.This is the triage plan on #2955, simplified after an adversarial re-derivation against the current code (no new marker kinds, no alias defs, no name-hiding rule) and with the hoisted run filtered to the defs reached by name from the main filter, which a review of the closure found must retry an imported module's bare sibling calls under its alias (pinned).
Measured (release, Apple M-series,
/usr/bin/time -l, the issue's generator; output identical to jq on every row)Processed-program node count at 6 / 8 / 10 levels: 1253 / 5093 / 20453 before, 169 / 241 / 313 after (
link_size_guard_2955, which fails against the old loader). Recursion ceiling through a linked dependency: 19494 before and after.M4 Pro, idle, interleaved, 25 reps, medians:
[range(1e5) | f]withfcalling one dependency through a stub +1.6%, against +3.3% drift on the same defs inline (neutral within noise). A 20-module x 50-def chain with one def used: 20 ms → 9 ms. The shape that pays: the same chain with all 50 top-level defs used links all 950 dependency defs into the top-level chain: 32 ms → 136 ms, because every chain def is installed over the whole program below it at bind time — the evaluator's pre-existingO(M x N)(a single 3000-defincludecosts 1 GB / 0.5 s today). Recorded in limitations.md and filed with #3148.What remains above jq's 2 MB is the evaluator's per-call bound-body retention (#3148, filed from this: the same 56 defs in one file cost 58 MB,
fib(20)234 MB,fib(21)overflows the stack). #3149 records a$param-generator divergence found while pinning rows; it predates this change and is not touched.Behaviour changes, all captured from
/usr/bin/jq1.7.1import "inner3" as i; def h: i::k;inside a module, wherekcalls its sibling bare, now answers[42](wasg/0 is not defined).All 30 rows of
test_dependencies_are_bound_in_their_own_scope_2962and the #2865/#2951/#2989/#2774/#2857/#2991/#2740/#2971/#2395/#2682 module families are unchanged.Verification
cargo fmt --check, both ci.yml clippy sets,cargo doc --all-features,cargo test --features cli,cargo test,cargo test --no-default-features— all green on the rebased tree.