Skip to content

Pool environments for direct-recursive functions (fib(30): -99.75% allocation)#2549

Merged
lahma merged 1 commit into
sebastienros:mainfrom
lahma:okojo-ws3-recursion-pool
Jun 27, 2026
Merged

Pool environments for direct-recursive functions (fib(30): -99.75% allocation)#2549
lahma merged 1 commit into
sebastienros:mainfrom
lahma:okojo-ws3-recursion-pool

Conversation

@lahma

@lahma lahma commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator

Summary

A direct-recursive function (fib, ackermann, tree-walks…) could not benefit from the per-function env/slot reuse pool. The pool has a single cached slot, but recursion keeps several frames live at once, so one slot can only ever serve the topmost frame — every recursive call allocated a fresh FunctionEnvironment + Binding[]. fib(30) alone allocated 412 MB.

This adds a small bounded, best-effort reuse pool per JintFunctionDefinition.State, used only for the direct-recursive, non-escaping case:

  • Each returned env is pooled with its cleared fixed-slot Binding[] still attached, so a sibling recursive frame rents back both the env and its slots.
  • The pool is a fixed slot-array with per-slot Interlocked (so it stays safe for the parallel Test262 fixtures that share a cached State), plus an occupancy-count hint so the two hot cases — pool empty during a deep descent and pool full during a deep unwind — skip the slot scan entirely.
  • A dedicated NewRecursiveFunctionEnvironment handles the recursive path; the non-recursive single-slot path is byte-for-byte unchanged, so non-recursive calls cannot regress.

Correctness invariant: a pooled env is removed from the pool while rented and only returned when its frame completes, so no two simultaneously live frames ever share an env. Escaping (closure-capturing) recursive functions keep EnvironmentMayEscape, so they are never pooled.

Benchmark

New Jint.Benchmark/RecursionBenchmark (fresh engine per invocation, default BDN job, .NET 10, Ryzen 9 5950X, same-runtime worktree A/B):

Benchmark Baseline This PR Δ time Δ alloc
Fib(30) — wide 619 ms / 412 MB 528 ms / 1.04 MB −17% −99.75%
Tak(18,12,6) — wide 16.7 ms / 12.9 MB 14.4 ms / 0.34 MB −14% −97.4%
DeepSum(400) — linear 227 ms / 197 MB 234 ms / 197 MB +2.8% flat

Wide branching recursion (the common shape, and the one the pool targets) sees near-total allocation elimination and a double-digit time win. Deep linear recursion whose depth far exceeds the pool cap can't reuse (all frames are live at peak), so its allocation is unchanged and it pays a small (~3%, within the noise band) cost for the pooling attempts.

Non-recursive call benchmarks (ClosureCallBenchmarks, MethodCallBenchmark, FunctionInvocationBenchmarks): allocation flat, time within run-to-run noise — the non-recursive path is unchanged.

Correctness

  • dotnet test Jint.Tests: 3138 passed, 0 failed.
  • dotnet test Jint.Tests.PublicInterface: 79 passed, 0 failed.
  • Test262 (strict + sloppy, parallel fixtures share cached State — exercises the pool's thread-safety): 0 new failures.
  • Ad-hoc checks: recursive frame/local isolation, escaping-closure recursion (must not pool), exception unwinding then reuse, ackermann, mutual recursion, and deep recursion past the pool cap.

🤖 Generated with Claude Code

@lahma @claude
Pool environments for direct-recursive functions
f518a38 
A direct-recursive function (fib/ackermann/tree-walks) could not use the single-slot env/slot
pool: with several frames live at once, one cached slot only serves the topmost frame, so every
recursive call allocated a fresh FunctionEnvironment + Binding[]. fib(30) alone allocated 412 MB.

Add a small bounded, best-effort reuse pool per JintFunctionDefinition.State for the
direct-recursive, non-escaping case. Each returned env is pooled with its cleared fixed-slot
Binding[] still attached, so a sibling recursive frame rents back both env and slots. The pool is
a fixed slot-array with per-slot Interlocked (safe for the parallel test fixtures that share a
cached State) plus an occupancy-count hint so the common "pool empty during descent" and "pool
full during a deep unwind" cases skip the scan. NewRecursiveFunctionEnvironment handles the
recursive case; the non-recursive single-slot path is byte-for-byte unchanged.

RecursionBenchmark (fresh engine, default job, .NET 10, same-runtime A/B):
  Fib(30):      619 ms / 412 MB  ->  528 ms / 1.0 MB   (-17% time, -99.75% alloc)
  Tak(18,12,6): 16.7 ms / 12.9 MB ->  14.4 ms / 0.34 MB (-14% time, -97.4% alloc)
  DeepSum(400): 227 ms / 197 MB  ->  234 ms / 197 MB   (+2.8% time, flat — depth far exceeds the
                                                         pool cap so linear recursion can't reuse)
Non-recursive call benchmarks: allocation flat, time within noise (path unchanged).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@lahma
lahma merged commit 3c66477 into sebastienros:main Jun 27, 2026
4 checks passed
@lahma
lahma deleted the okojo-ws3-recursion-pool branch June 28, 2026 08:43
This was referenced Jun 30, 2026
Merged
Merged
lahma added a commit to lahma/jint that referenced this pull request Jul 3, 2026
@lahma @claude
Move the function-env reuse pool off the shared State to the function…
54878cd 
… instance (sebastienros#2560)

Since sebastienros#2413 a call's FunctionEnvironment is pooled on the shared
JintFunctionDefinition.State. An environment roots its creating engine
(Environment._engine plus its outer chain to the global), and a prepared
script's State is shared by every engine that runs it — so the pool slot
pins the last-caller engine (and its realm) until the function is next
invoked by any engine. For idle functions in long-lived cached prepared
scripts the engine is pinned indefinitely (issue sebastienros#2560, secondary cause).

Move the pool (single cached env, cached slot array, and the bounded
recursive pool from sebastienros#2549) onto the ScriptFunction instance. Function
instances are created per engine, so the pool is per-engine by construction
and dies with the engine — while same-instance call reuse and recursive
frame reuse, the workloads the pools exist for, are unchanged. Because a
function instance is single-threaded like its engine, the Interlocked
handshakes and cross-engine identity checks the shared-State pool needed
are no longer necessary.

Includes a WeakReference regression test: 20 throwaway engines running one
shared prepared script must all be collectable (fails before this change —
the last engine stayed pinned).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bf1LESD4X8dMNPwLKJCAh9
lahma added a commit to lahma/jint that referenced this pull request Jul 4, 2026
@lahma @claude
Move the function-env reuse pool off the shared State to the function…
1680ffd 
… instance (sebastienros#2560)

Since sebastienros#2413 a call's FunctionEnvironment is pooled on the shared
JintFunctionDefinition.State. An environment roots its creating engine
(Environment._engine plus its outer chain to the global), and a prepared
script's State is shared by every engine that runs it — so the pool slot
pins the last-caller engine (and its realm) until the function is next
invoked by any engine. For idle functions in long-lived cached prepared
scripts the engine is pinned indefinitely (issue sebastienros#2560, secondary cause).

Move the pool (single cached env, cached slot array, and the bounded
recursive pool from sebastienros#2549) onto the ScriptFunction instance. Function
instances are created per engine, so the pool is per-engine by construction
and dies with the engine — while same-instance call reuse and recursive
frame reuse, the workloads the pools exist for, are unchanged. Because a
function instance is single-threaded like its engine, the Interlocked
handshakes and cross-engine identity checks the shared-State pool needed
are no longer necessary.

Includes a WeakReference regression test: 20 throwaway engines running one
shared prepared script must all be collectable (fails before this change —
the last engine stayed pinned).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bf1LESD4X8dMNPwLKJCAh9
lahma added a commit to lahma/jint that referenced this pull request Jul 4, 2026
@lahma @claude
Move the function-env reuse pool off the shared State to the function…
ffe6157 
… instance (sebastienros#2560)

Since sebastienros#2413 a call's FunctionEnvironment is pooled on the shared
JintFunctionDefinition.State. An environment roots its creating engine
(Environment._engine plus its outer chain to the global), and a prepared
script's State is shared by every engine that runs it — so the pool slot
pins the last-caller engine (and its realm) until the function is next
invoked by any engine. For idle functions in long-lived cached prepared
scripts the engine is pinned indefinitely (issue sebastienros#2560, secondary cause).

Move the pool (single cached env, cached slot array, and the bounded
recursive pool from sebastienros#2549) onto the ScriptFunction instance. Function
instances are created per engine, so the pool is per-engine by construction
and dies with the engine — while same-instance call reuse and recursive
frame reuse, the workloads the pools exist for, are unchanged. Because a
function instance is single-threaded like its engine, the Interlocked
handshakes and cross-engine identity checks the shared-State pool needed
are no longer necessary.

Includes a WeakReference regression test: 20 throwaway engines running one
shared prepared script must all be collectable (fails before this change —
the last engine stayed pinned).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bf1LESD4X8dMNPwLKJCAh9
lahma added a commit that referenced this pull request Jul 4, 2026
@lahma @claude
Per-engine environment caches: prepared scripts no longer pin engines…
eceb7ba 
…; computed-key and escape-analysis fixes (#2568)

* Run deep call-stack test on a dedicated large-stack thread

ShouldAllowReasonableCallStackDepth nests 990 calls (Release) on a default
engine, which has no stack guard (MaxExecutionStackCount is disabled), so
the depth runs directly against the native stack. The xUnit worker thread's
remaining stack varies per run, making the test crash the whole process
intermittently with STACK_OVERFLOW (0xC00000FD) — reproducible on unmodified
main on Windows/net10.0.

Run the body on a dedicated thread with an explicit 16 MB stack so the
available stack is deterministic.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bf1LESD4X8dMNPwLKJCAh9

* Move the function-env reuse pool off the shared State to the function instance (#2560)

Since #2413 a call's FunctionEnvironment is pooled on the shared
JintFunctionDefinition.State. An environment roots its creating engine
(Environment._engine plus its outer chain to the global), and a prepared
script's State is shared by every engine that runs it — so the pool slot
pins the last-caller engine (and its realm) until the function is next
invoked by any engine. For idle functions in long-lived cached prepared
scripts the engine is pinned indefinitely (issue #2560, secondary cause).

Move the pool (single cached env, cached slot array, and the bounded
recursive pool from #2549) onto the ScriptFunction instance. Function
instances are created per engine, so the pool is per-engine by construction
and dies with the engine — while same-instance call reuse and recursive
frame reuse, the workloads the pools exist for, are unchanged. Because a
function instance is single-threaded like its engine, the Interlocked
handshakes and cross-engine identity checks the shared-State pool needed
are no longer necessary.

Includes a WeakReference regression test: 20 throwaway engines running one
shared prepared script must all be collectable (fails before this change —
the last engine stayed pinned).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bf1LESD4X8dMNPwLKJCAh9

* Keep slot arrays shared on State; cache the env directly on the instance

Refine the per-instance env cache based on A/B benchmarking:

- Cleared fixed-slot Binding[] hold no engine references, so restore them to
  the shared State cache (exactly as before) — freshly created instances of
  the same function (e.g. a prepared script re-evaluated, or a function
  expression re-created per loop iteration) reuse the slot array again.
- Store the cached FunctionEnvironment directly in a single object field on
  ScriptFunction, interpreted via State.IsDirectRecursive (direct-recursive
  functions store the bounded RecursiveEnvPool there instead). This drops
  the pool wrapper allocation and keeps the instance-size cost at one field.

vs main: recursion and closure benchmarks are allocation-identical and
3-13% faster (the env-side Interlocked handshake is gone); the remaining
deltas are the leak fix itself — one env allocation per re-created-and-
called function instance (~0.1 KB) and 8 bytes per ScriptFunction.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bf1LESD4X8dMNPwLKJCAh9

* Apply code-review findings: scan parameter expressions in escape analysis

The environment-escape analysis (EnvironmentEscapeAstVisitor.MayEscape)
scanned only the function body, so a closure created in a parameter default
expression escaped undetected and the call's environment was cached for
reuse while still reachable from the closure — the next call's Reset then
rebound the bindings the escaped closure resolves through:

  function f(a, get = function() { return a; }) { return get; }
  var g1 = f(1); f(2);
  g1(); // returned 2 instead of 1 (pre-existing before this branch too)

Scan function.Params as well; the slot-aware MayEscapeWithReferences path
needs no change because fixed slots require !HasParameterExpressions.

Also from the review:
- RecursiveEnvPool: plain LIFO stack pointer instead of the O(16) slot scans
  inherited from the lock-free shared-State pool (single-threaded now);
  fib -9.5% vs main (was -5.0%)
- GC engine-retention test: cover the direct-recursive pool path too, and
  NoInlining on the helper so the engine can't be stack-rooted; document
  that block/loop environments cached on shared AST state can still pin the
  last-caller engine (pre-existing, tracked separately)
- EngineLimitTests: 4 MB dedicated stack instead of 16 MB so a large
  per-frame native-stack regression still fails the test, and drop the
  macOS skip (the explicit stack is platform-independent)
- hoist the duplicate (ScriptFunction) cast; fix stale comments describing
  the deleted shared-State pool

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bf1LESD4X8dMNPwLKJCAh9

* Apply second-review findings: this/new.target capture, construct rent gate

- ClosureReferencesAny (the slot-aware escape analysis) only matched
  identifiers against slot names, so an escaping closure whose only lexical
  dependency is `this`/`new.target`/`super` was classified non-capturing and
  its FunctionEnvironment was cached and rebound by the next call:

    function f(a) { var g = function() { return 1; }; var h = () => this; return h; }
    var h1 = f.call({name:'o1'}, 1); f.call({name:'o2'}, 2);
    h1().name; // returned 'o2' — also reproducible on main with the old State cache

  The environment carries the this-binding, new.target and super base, so
  any such node inside a closure now counts as an environment reference
  (conservative). Regression tests for the this and new.target variants.

- The construct path rents from the reuse cache via PrepareForOrdinaryCall
  but never returns its environment, so every `new f()` drained the cache
  and starved the next call's reuse (upstream parity). Rent only for plain
  calls (newTarget undefined).

- EngineLimitTests: back to a generous 16 MB dedicated stack —
  StackOverflowException is uncatchable and would kill the whole test
  process on platforms with larger native frames; the test is a functional
  smoke test of deep call chains, not a per-frame stack-budget test.

Benchmarks after these fixes (vs main): Fib -10.3%, ParamLocalCall -12.7%,
closures -6..7%, Warm_* -3..6%, allocations unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bf1LESD4X8dMNPwLKJCAh9

* Cache the block environment on the statement handler, not the shared BlockState

Same #2560 engine-retention family as the function-env cache: BlockState
lives on the AST node's UserData and is shared by every engine running a
prepared script, and the DeclarativeEnvironment cached on it roots its
creating engine — so any prepared script containing a let/const block kept
the last-caller engine (and its realm) alive. Reproduced: 1 of 20 throwaway
engines stayed pinned.

Move the cache to the JintBlockStatement handler instance, which is built
per statement list and therefore per engine; the Interlocked handshake and
engine-identity check become unnecessary. BlockState keeps only the
immutable, engine-agnostic data (declarations, slot names/templates).

JintForStatement._cachedLoopEnv needed no change: statement handlers are
never cached on AST UserData (only BlockState is), so the loop-env cache
was already per engine — confirmed by the extended regression test, which
covers an ordinary function, a direct-recursive one, a let/const block and
a for-let loop and requires all 20 engines to be collectable.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bf1LESD4X8dMNPwLKJCAh9

* Evaluate any expression type in computed property keys

AstExtensions.TryGetComputedPropertyKey had a node-type allowlist with a
silent JsValue.Undefined fallback: computed keys whose expression type was
missing from the list (SequenceExpression, NewExpression,
TaggedTemplateExpression, ChainExpression, ClassExpression, ...) bound the
property "undefined" and never evaluated the key expression at all — side
effects were silently skipped:

  var calls = 0;
  function fb(x, { [(calls++, "k")]: v }) { return v; }
  fb(1, { k: 6 }); // returned undefined with calls === 0

Every expression type is buildable by JintExpression.Build and both call
sites already handle abrupt completions and generator suspension after
evaluating the key, so drop the allowlist and always evaluate.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bf1LESD4X8dMNPwLKJCAh9

* Apply third-review findings: evaluate decorated computed keys exactly once

- Decorator bookkeeping re-called GetKey after the class element was already
  defined, re-running computed key side effects and — when the key changed
  between evaluations — silently applying the decorator to the wrong
  (nonexistent) property. Thread the defined key out of
  MethodDefinitionEvaluation/ClassElementEvaluation, reuse
  ClassFieldDefinition.Name for fields and carry the declared key in
  AutoAccessorResult (FieldDefinition.Name is the private backing name).
  Pre-existing for previously-allowlisted key types; became more visible
  once computed keys evaluate all expression types.

- Object literals: check for generator suspension after evaluating a
  computed method key (mirrors the non-method path) so the build loop
  doesn't keep executing properties while the generator is suspended.

- Block env cache: park only after leaving the block, and reset slots +
  detach the outer chain at park time so the cached env doesn't root the
  completed call's scope chain and last binding values until reuse.

- Remove the write-only BlockState.CanReuseEnvironment; document the
  handlers-must-not-be-shared invariant at JintStatement.Build, where a
  future UserData caching change would violate it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bf1LESD4X8dMNPwLKJCAh9

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 6, 2026
Closed
Closed
Closed
Merged
Closed
Open
Merged
This was referenced Jul 13, 2026
Closed
Closed
Open
Open
Closed
@dependabot dependabot Bot mentioned this pull request Jul 20, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@lahma