ReviewOS

zig-utils/zig-js

Runtime: private environment flattening copies every enclosing name per class evaluation

#927
Closed glennmichael123 opened this 8 days ago ·
8 days ago

Parent: #493 Related: #926 (the parse-time instance of the same shape), #467

Status: code inspection, verified by reading. No measurement exists yet and no performance claim is made here.

#926 measured and fixed the parse-time copy of inherited private names. While tracing its blast radius I found the same shape at runtime, in two places in src/interpreter.zig. Neither is affected by the #926 fix, and together they bound how much that fix can be worth end-to-end: a source whose parse is now linear still pays a quadratic cost per class evaluation.

1. Published private maps are flattened per class (src/interpreter.zig:8492-8499)

std.debug.assert(map.count() != 0);
// Keep the published map immutable. Its flattened lookup preserves the
// complete lexical private environment without mutating an outer class.
if (self.current_private_map) |outer| {
    var names = outer.iterator();
    while (names.next()) |entry| {
        if (!map.contains(entry.key_ptr.*))
            try map.put(self.arena, entry.key_ptr.*, entry.value_ptr.*);
    }
}

Every class's published map absorbs a full copy of its enclosing environment. For N nested classes over N names that is N x N entries, and unlike the parser's version these maps are published and long-lived on self.arena, so the cost is retained rather than transient.

To be fair to the code: the comment says this is deliberate. Flattening buys O(1) private lookup instead of walking a scope chain, and immutability is what keeps an inner class from mutating an outer one. This issue is not "this is a bug" — it is that the trade-off is unbounded on attacker-controlled nesting, which is exactly the class #493 exists to bound.

2. Rewrite clones the map per nested class (src/interpreter.zig:8205-8214)

.class_expr => |c| {
    if (c.superclass) |sc| try self.rewritePrivateNamesInNode(sc, map);
    var nested_map = map.emptyClone();
    defer nested_map.deinit(self.arena);
    var it = map.iterator();
    while (it.next()) |entry| try nested_map.put(self.arena, entry.key_ptr.*, entry.value_ptr.*);

Same copy-per-nested-class shape. This one does free (defer ... deinit), so it is quadratic in time but not in retained memory — strictly less severe than 1.

Why the #926 fix does not address these

The parser's set answers "is this name in scope" and stores void, which is what let an undo log replace the copy there. These maps carry a storage key per name, and shadowing is resolved by removing the inner class's own names from the clone (interpreter.zig:8212) rather than by membership. The value payload and the inverted shadowing rule mean the parser's undo-log trick does not port over directly.

Suggested order

  1. Freeze runtime growth workloads the way #926 did for the frontend, including a control that evaluates the same class count with nothing to inherit. Measure before proposing anything — I have deliberately not claimed a curve here.
  2. Only then weigh a scope-chain with a lookup cache against the current flattening, keeping the immutability guarantee that makes inner classes unable to mutate an outer environment.

Non-goals

No nesting limit, no weakening of private-brand semantics, and no published benchmark claim until dated evidence lands under docs/.data/.

8 days ago

Measured (#933 item 7)

Both curves are confirmed, and the nested one is worse than this issue predicted: cubic, not quadratic.

Programs evaluate every class at definition time by nesting through static field initializers, so class evaluation is what's measured, not parsing. Each shape has a control with the same class count and nothing to inherit. test262 --eval, peak RSS from /usr/bin/time -l.

Siblings — one outer class with N private names and N inner classes (ReleaseFast, b1acc36 — the flattening code is unchanged since):

N256512102420484096
siblings20 MB40 MB108 MB371 MB1,186 MB / 1.17 s
control (no inherited names)14 MB17 MB23 MB35 MB61 MB

x3.3–3.7 per doubling: quadratic, as site 1 above predicts.

Deep — N nested classes, one private name each (same binary):

N6496128160192256512
deep18 MB33 MB63 MB112 MB182 MB411 MB>3 GB (guard killed it)
control11 MB12 MB12 MB13 MB13 MB——

About x8 per doubling: cubic.

Where the cubic comes from — tested, not read

The rewritePrivateNamesInNode clone at site 2 is released with nested_map.deinit(self.arena), and an arena does not free individual allocations — so the clone this issue describes as "quadratic in time but not in retained memory" is retained. Each class evaluation rewrites its remaining nested subtree, cloning the map at every nested class, which is Σ(N−k)² entries across the chain.

To check that rather than assert it, I built a throwaway variant whose only change is giving that clone a truly freeing allocator (Debug builds on both sides, same source otherwise):

deep N6496128192256
main, peak RSS28 MB44 MB74 MB195 MB426 MB
clone freed, peak RSS22 MB24 MB27 MB34 MB43 MB
main, time0.15 s0.18 s0.33 s0.95 s2.31 s
clone freed, time0.83 s0.23 s0.41 s1.18 s2.61 s

So the cubic memory is exactly the retained clones: 10x less at 256 once they can be freed. But time stays cubic, because the cloning work itself is repeated at every nested class on every evaluation pass.

What that means for the fix

  • Freeing the clone alone is not the fix. It removes the retained cubic but not the repeated work, and the idiomatic transient allocator (scratch_allocator orelse gc.temporaryAllocator(arena)) falls back to the arena when no heap is active, so it would not even reliably free. The experiment used page_allocator only to prove attribution; nothing from it is committed.
  • The real fix is the design question already raised here, now with a sharper target: avoid re-cloning the inherited map per nested class per pass, and avoid flattening every enclosing name into each published map — while keeping the immutability that stops an inner class from mutating an outer one.
  • Generators for all four shapes, ready to freeze as runtime growth rows when that fix is written:
    • deep: class C{i} { #n{i} = {i}; static inner = × N, then 0, then ; } × N
    • deep control: the same without the private field
    • siblings: class Outer { #n0 … #n{N-1}; static f{i} = class { g() { return 0; } }; × N }
    • siblings control: the same without the private fields
8 days ago

Fixed on main by 9c4db51.

Each evaluated class now publishes only its own private bindings and an immutable parent pointer. Lookup walks that lexical chain, so shadowing remains local and inner classes cannot mutate outer environments. Private-name rewriting and AST copying stop at nested-class boundaries; each nested class copies and resolves only its own body when its ClassDefinitionEvaluation runs. This removes both the flattened long-lived maps and the repeated descendant rewrite/copy passes.

V38 freezes four diagnostic families in bench/private_environment_growth.js. Source generation and Function parsing happen during workload selection; each scored logical job evaluates the same AST once. Each subject has a control with the same class count, static-initializer work, checksum, mode, and lane.

Exact-parent/candidate results on this host, two scored samples per binary and width, with /usr/bin/time -l process peak RSS:

workloadparent mediancandidate medianparent peakcandidate peak
deep 320.453 ms0.227 ms32.3 MiB20.4 MiB
deep 642.864 ms0.280 ms109.6 MiB23.8 MiB
deep 12817.628 ms0.589 ms554.3 MiB30.6 MiB
siblings 2562.503 ms1.456 ms109.1 MiB48.2 MiB
siblings 51217.006 ms4.520 ms316.7 MiB71.4 MiB
siblings 102470.377 ms11.949 ms389.1 MiB118.5 MiB

The controls stay in the same memory band before and after. Every parent/candidate checksum matches, and Node produces the same checksum for all 12 frozen subject/control rows. The larger one-shot diagnostic from the earlier comment also changes from 411 MiB at deep 256 to 15.1 MiB versus a 13.9 MiB control, and deep 1,024 completes at 27.2 MiB versus a 21.7 MiB control.

Correctness and gates:

  • standalone private-focused suite: 49/49 pass, including tree-walker, VM, moving-GC, direct-eval, and shared no-GIL cases;
  • exact-parent test262: class expressions 4,059, class declarations 4,367, eval code 347; total 8,773 → 8,773 pass, 0 fail, with byte-identical diagnostics and 0 flips;
  • V38 representative-matrix validation passes; structural tests 75/75;
  • all 12 frozen workloads execute in the comparison runner and match Node checksums;
  • docs build: 50 pages, checked manifest passes.

The full benchmark-comparison-test continues to hit #933's existing default evaluation-step-budget blocker after the relevant benchmark, publication, matrix, attribution, instrumentation, build-feedback, performance-attribution, and exact-parent validators pass. No workload reduction or runtime-limit change was made.

Sign in to comment on this issue.