Add Op::CallChildren: eliminate children()'s native reentry - #66
Merged
Conversation
The cache was keyed by the list's first element alone -- a convention
borrowed from assignBlockChunkCache_, where it IS sufficient (a leading-
assignment run's first member uniquely determines the run). Here it
wasn't: children()'s forwarding produces two DIFFERENT lists sharing a
first element (bare children() forwards the caller's whole list;
children(0) a single-element slice of it), and whichever form ran first
poisoned the cache for the other. Concrete failure:
`module m() { children(); children(0); }` called with two children made
children(0) emit BOTH shapes; reversed statement order truncated bare
children() instead.
Re-keyed by (front, size). Every evalChildren list producer enumerated:
fixed AST lists have unique fronts per call site, and the only same-
front pair (bare vs indexed forwarding) always differs in size except
when the lists are literally identical anyway. Two regression tests
(both statement orders), each verified to fail before the re-key.
Also splits the lookup half of tryRunCompiledChildren into
lookupOrCompileChildrenListChunk, needed shortly by Op::CallChildren's
own runtime handler (same list->chunk step, different run mechanism).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…xed-trail out-of-order pop children() was the last dominant source of native C++ stack reentry from compiled code (85 of 93 native-reentry hits in Anklet.scad's real run) -- its statement fell to Op::NativeStatement, whose handler re-entered the VM through six native frames per children() per recursion level, and BOSL2's attachable() calls it at nearly every wrapper level. Unlike Op::PushBuiltinWrap's constructs, the forwarded children are runtime-varying (ctx.childrenNodes/childrenCallerCtx, set by buildModuleChildCtx from the CALLER's call-site AST), so they can't compile inline. Op::CallChildren instead resolves the list at runtime, looks up/compiles its chunk (the same pass-scoped childrenListChunkCache_ tryRunCompiledChildren uses), and pushes it onto vmCallStack_ directly -- mirroring Op::CallModule's zero-native-call push, but with a third frame shape: splice-owning (ownsModuleSplice=true, matching evalModularCall's unconditional splice branch for "children") yet bracketless (children() never gets a callStack_/profiling entry natively either). The completion branch and exception teardown already handle that combination unchanged. The "which nodes, evaluated against what context" half of builtinChildren (the caller-ctx re-derivation, the $-forwarding loop, the children(N) index filtering) is factored into a shared prepareChildrenForward helper so the native and compiled paths cannot drift. Handler ordering is load-bearing: checkDebug against the scope-wrapped ctx (byte-for-byte what Op::NativeStatement did for this node), randsBefore before argument resolution (rands-in-args taint), treeStack_ pushed last (exception safety without a try/catch), and a not-eligible fallback that reuses the already-resolved args instead of re-resolving via evalStatement (which would double rands()/echo/assert side effects). Also fixes a REAL latent bug this surfaced: IndexedScopeTrailStorage:: popLevel did a blind pop_back() per dirty name -- the exact out-of-order- pop corruption its non-indexed twin's own doc comment describes and fixed long ago -- benign only while every dyn-trail view died LIFO. The new forwarding frame's evalCtx (opened after, dying after, the call's own effCtx) is the first real non-LIFO lifetime: popping effCtx's level silently ate the still-live forwarded entry on top (caught for real: children($fn=9) read back as the root default 0 inside the forwarded child). Now level-aware, same as the twin, with a storage-level regression test mirroring the exact level shape. Verification: all 721 tests green on both OSCAD_BYTECODE_VM states; new tests cover the recursive wrap/children() chain at depth 1500 (bare and indexed), $-forwarding parity (wrapper writes and named-$ args), the trail pop fix, and (previous commit) the (front,size) cache re-key. Anklet.scad now renders end-to-end with fully unmodified guards -- 315984 bytes, byte-identical to the disabled-guards reference -- and its peak native reentry depth measured 10 (was 55 originally, 45 after Op::PushBuiltinWrap), 4x inside the Windows-proven-safe ceiling of 40. Bumps version to 0.13.4. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
children()fell toOp::NativeStatement, costing one genuine native C++ reentry chain per call per recursion level — 85 of 93 native-reentry hits in Anklet.scad's real run, and the reason it still failed with "Recursion too deep (native call stack)" on 0.13.3 despite PR Close the NativeStatement gap for translate/color/modifier-wrapped recursion #63/Decouple kMaxUserCallDepth's guard from callStack_.size() #65.Op::CallChildren: unlike the PushBuiltinWrap constructs, the forwarded children are runtime-varying (ctx.childrenNodes/childrenCallerCtx), so they can't compile inline. The new opcode resolves the list at runtime and pushes its chunk ontovmCallStack_directly (mirroringOp::CallModule's zero-native-call push) with a third frame shape: splice-owning yet bracketless — both already handled unchanged by the completion branch and exception teardown.builtinChildrenis factored into a sharedprepareChildrenForwardhelper so native and compiled paths cannot drift ($-forwarding loop, caller-ctx re-derivation,children(N)filtering).childrenListChunkCache_keyed by list-front alone —children()andchildren(0)on the same caller shared a cache key, making whichever ran first poison the other (wrong geometry emitted, both statement orders). Re-keyed by(front, size). (Own commit.)IndexedScopeTrailStorage::popLeveldid a blindpop_back()per dirty name — the exact out-of-order-pop corruption its non-indexed twin documents and fixed long ago, benign only while dyn-trail view lifetimes stayed LIFO. The new forwarding frame is the first real non-LIFO lifetime; achildren($fn=9)override silently read back as 0. Now level-aware, same as the twin.Results
OSCAD_BYTECODE_VMstates.wrap() { children(); }chain at depth 1500 (bare + indexed), $-forwarding parity (wrapper$fnwrites andchildren($fn=N)named args), the trail-pop fix, the cache-key collision (both orders).Test plan
🤖 Generated with Claude Code