ReviewOS

zig-utils/zig-js

Perf: nested destructuring catch parameters are quadratic inside function bodies

#932
Closed glennmichael123 opened this 9 days ago ·
9 days ago

Parent: #493 Related: #928 (whose benchmark could not see this), #931 (where it surfaced)

Status: measured end to end, root cause not yet located. No fix proposed.

Found while confirming that the #931 fix did not introduce a quadratic. It did not — but the shape it was tested on already was one, in the binary from before that fix.

Evidence

Median of 5 runs of test262 --eval, which covers process start, parse and function instantiation. Each shape nests to the given depth; the inside-a-function shapes wrap everything in function __never(){ … }, which is never called, so nothing executes.

shapedepth 1000depth 2000depth 4000growth
destructuring catch ([p]), inside a function, with vars123.2 ms481.8 ms1865.3 msx3.91, x3.87
destructuring catch ([p]), inside a function, no vars122.8 ms475.9 ms1863.3 msx3.88, x3.92
plain catch (p), inside a function7.4 ms8.3 ms10.4 mslinear
destructuring catch ([p]), top level, with vars8.6 ms10.5 ms16.0 mslinear
plain blocks with vars, inside a function14.8 ms36.0 ms145.8 msx2.44, x4.05

The same measurement on the destructuring-with-vars shape gives nearly identical numbers before and after #931 (1871 ms vs 1977 ms at depth 4000), so this predates that fix.

What the isolation rules out

  • Not the vars: removing every var changes nothing (1865 vs 1863 ms).
  • Not catch semantics in general: plain catch (p) is linear in the same position.
  • Not the parser's handling of this syntax as such: the identical nesting is linear at script top level.
  • Not #931's new check: present before it, and the fix adds only a few percent.

What remains is the conjunction: destructuring catch patterns, inside a function body. Something that processes function bodies does per-level work proportional to the nested destructuring patterns below it. Plausible places, unverified: a per-function pass that re-scans nested catch patterns at each level (scanSuperAndArgs does visit catch_param via scanSuperAndArgsInPattern), function-body eligibility or scope analysis at instantiation, or binding collection for catch patterns.

A caveat this exposes about #928

The #928 frozen workloads — like #926's — parse top-level scripts. They measured and fixed a real quadratic there, and that result stands. But a benchmark built on top-level programs cannot see an effect that only occurs inside function bodies, and the last row above suggests one exists for plain nested blocks too, still steepening at depth 4000. So "#928 removed the nesting quadratic" should be read as scoped to top-level code until function-body shapes are measured.

Suggested order

  1. Add function-body variants to the frozen growth runner — each #926/#928 shape wrapped in a function, plus the destructuring-catch shape here — so the effect is measured by the owned runner and not only by --eval wall time.
  2. Sample a depth-4000 run (sample <pid> 3) to locate the hot frames before guessing.
  3. Only then propose a fix.
9 days ago

Fixed in 2688dc9

Root cause — located by sampling, not guessed

A depth-10000 run sampled with sample put essentially all the time in the bytecode compiler, not the parser:

compileStmt (recursing through each try/catch) → compileCatchPattern → compileArrayPattern → emitLoad → resolve

with top-of-stack compiler.Compiler.resolve (969 samples), hash.wyhash.Wyhash.hash (431) and compiler.Compiler.withDepthToResolution (339). That explains the whole isolation table from the original report: an uncalled function's body is still compiled, while top-level code in these probes was not.

The cost was in FnScope.get:

var index = self.lexical_scopes.items.len;
while (index > 0) {                    // probes EVERY open lexical scope...
    index -= 1;
    if (self.lexical_scopes.items[index].get(name)) |binding| return binding;
}
if (self.names.get(name)) |binding| return binding;   // ...before the function-level map

Lexical scopes are a stack inside one FnScope, not separate entries in the parent chain, so resolve's outer loop runs once but get searches the whole stack. Activation temps are added to names by addLocal, so every temp lookup missed in all N catch scopes first — each a real wyhash, because a catch scope holds its parameter and is not empty. A destructuring pattern's iterator protocol performs many temp loads and stores, each resolved up to three times. A plain catch (p) touches a temp once or twice, which is why its quadratic term was invisible at these depths.

The fix

FnScope counts how many open lexical scopes bind each name, and get skips the probe when that count is zero — a name no open scope binds cannot be found there, so every result is unchanged. Counts rise before a binding is inserted and fall only on pop, so they can over-count but never under-count.

I deliberately did not make temps bypass name resolution by slot. Temps go through the with and direct-eval dynamic binding plans, and short-circuiting that would change semantics inside with rather than just speed.

Results — median of 5 --eval runs

shapedepthbeforeafter
destructuring catch, in function1000122.9 ms12.1 ms
2000494.1 ms18.3 ms
40001895.0 ms30.2 ms (63x)
plain catch, in function400011.1 ms10.4 ms
destructuring catch, top level400013.1 ms12.9 ms
plain blocks with vars, in function4000155.5 ms164.2 ms

Growth for the fixed shape is now x1.51 and x1.65 per doubling (flattened by process start).

A correction to my own report above: I suggested the steepening of plain nested blocks inside a function might be the same loop. It is not — this fix leaves that shape unchanged, because Zig's hash map returns before hashing on an empty map, and plain block scopes are empty. It has a separate cause and is tracked in #933 item 2.

Verification

Because this changes name resolution for every compiled function, and ordinary test262 runs mostly use the tree-walker, the corpus was run with the bytecode path forced:

  • --vm-witness-subtree, ReleaseFast, 23 subtrees weighted toward with, eval, identifier resolution and scoping — 13,814 tests, divergence sets identical to the pre-change binary in every subtree. The pre-existing divergences in expressions/call (3), statements/try (3), statements/switch (3) and statements/for (4) are byte-identical before and after.
  • Invariant check. Soundness depends on every lexical insertion being counted, so in safe builds get re-probes each skip and asserts the name is really absent — exactly what get always cost before, so Debug is no slower. With it live: compiler tests 313 passed, 0 failed in Debug, and a ReleaseSafe --vm-witness-subtree run over 9 subtrees — 7,317 tests — exits cleanly with no panics.

Found on the way — #934

Timing at depth 8000 crashed both the pre- and post-fix binaries with SIGSEGV. Characterizing it showed a general bug: 32 KB of nested parentheses segfaults the process, where node throws a catchable RangeError: Maximum call stack size exceeded. Filed as #934 and placed right after this item in #933.

Sign in to comment on this issue.