You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Found by code review on PR #2954 (#2865). #2865 makes a module's own includes
load transitively by wrapping each exported def's body in the defs it reaches.
Because that wrap copies the AST, a chain of modules compounds: each level's
bodies already carry the level below.
#2865 mitigates this with jq's own block_bind_referenced rule (wrap only what
the body transitively calls), which flattens the common shape completely. It does not cure the general case: as soon as each def calls more than one def from
the level below, the referenced closure is itself exponential in chain depth.
Output is correct throughout — this is a memory/time scalability gap, not a
wrong answer, and not a regression (before #2865 these programs did not compile
at all).
Repro
Synthetic chains of modules, each def calling F defs from the level below,
resolving one def at the top. Apple M-series, release build, measured with /usr/bin/time -l at PR #2954's head:
Then succinctly jq -L <dir> -nc 'include "m13"; f13_0'.
Root cause
ModuleLoader::load_and_bind_module / visible_defs_for
(src/bin/succinctly/jq_runner.rs): binding is done by cloning each
reachable def's Expr into the wrapping body. Real jq binds symbolically — its block values are shared, so a def reachable by many paths exists once.
succinctly's module support is AST inlining, where "reachable by K paths" means
K copies.
Suggested fix direction
Three shapes, roughly in increasing order of cost:
Share the bound bodies.Expr already has an Rc-shaded variant for
exactly this purpose (the opaque already-substituted sub-expression jq: self-recursive user-defined functions can't work at any real depth under static AST substitution #1371
introduced to stop recursive calls growing with depth). If a bound def body
could be spliced by handle rather than by clone, the copies collapse to
pointers. Needs a check that every later pass over the tree treats it
correctly.
A depth/size guard that refuses with a clear error rather than
exhausting memory, in the spirit of the other resource caps in docs/compliance/jq/limitations.md. A stopgap, not a fix.
Option 1 is the one worth costing first; #2951's sealed-scope work would
subsume option 2.
#2865 is the functional gap (transitive includes not processed at all) and
ships the mitigation that handles the common shape; this is the residual
scalability limit of the mechanism it uses. #2951 is a scoping-correctness gap
whose fix would likely also change this mechanism, but it is not itself about
memory.
Severity: Low
Summary
Found by code review on PR #2954 (#2865). #2865 makes a module's own
includesload transitively by wrapping each exported def's body in the defs it reaches.
Because that wrap copies the AST, a chain of modules compounds: each level's
bodies already carry the level below.
#2865 mitigates this with jq's own
block_bind_referencedrule (wrap only whatthe body transitively calls), which flattens the common shape completely. It does
not cure the general case: as soon as each def calls more than one def from
the level below, the referenced closure is itself exponential in chain depth.
Output is correct throughout — this is a memory/time scalability gap, not a
wrong answer, and not a regression (before #2865 these programs did not compile
at all).
Repro
Synthetic chains of modules, each def calling
Fdefs from the level below,resolving one def at the top. Apple M-series, release build, measured with
/usr/bin/time -lat PR #2954's head:Generator:
Then
succinctly jq -L <dir> -nc 'include "m13"; f13_0'.Root cause
ModuleLoader::load_and_bind_module/visible_defs_for(
src/bin/succinctly/jq_runner.rs): binding is done by cloning eachreachable def's
Exprinto the wrapping body. Real jq binds symbolically — itsblockvalues are shared, so a def reachable by many paths exists once.succinctly's module support is AST inlining, where "reachable by K paths" means
K copies.
Suggested fix direction
Three shapes, roughly in increasing order of cost:
Expralready has anRc-shaded variant forexactly this purpose (the opaque already-substituted sub-expression jq: self-recursive user-defined functions can't work at any real depth under static AST substitution #1371
introduced to stop recursive calls growing with depth). If a bound def body
could be spliced by handle rather than by clone, the copies collapse to
pointers. Needs a check that every later pass over the tree treats it
correctly.
def — closer to jq's own block model, but it is the per-body wrapping that
buys jq: a module's own include directive is silently ignored (transitive include not processed) #2865 its scoping rows (a dependency visible inside the module, not
re-exported, outranking a same-name sibling), so this needs the scope
boundary from jq: a module body sees names it should not — ~/.jq's defs, and sibling included modules' defs in declaration order #2951 to exist first.
exhausting memory, in the spirit of the other resource caps in
docs/compliance/jq/limitations.md. A stopgap, not a fix.Option 1 is the one worth costing first; #2951's sealed-scope work would
subsume option 2.
Why this is separate from #2865, #2951
#2865 is the functional gap (transitive includes not processed at all) and
ships the mitigation that handles the common shape; this is the residual
scalability limit of the mechanism it uses. #2951 is a scoping-correctness gap
whose fix would likely also change this mechanism, but it is not itself about
memory.