From 353a377a594322a5cf1ae148049ba2e343763ae1 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Sun, 6 Sep 2026 09:51:45 -0700 Subject: [PATCH] Cache included ASTs in memory: a BOSL2 render goes 67ms -> 7.5ms Parsing `include ` is ~55ms, which was 82% of evaluating a small script against it -- and it happened on every render, of every script, even though the library never changed. An editor re-rendering paid it each time; BOSL2's docs build paid it once per example, a thousand times over. Files are now parsed once and shared (openscad_cpp_parser#7). The evaluator takes the borrowed statement list that produces, which needed a resolveUseScopes() overload for nodes it does not own, and Evaluator/ EvalContext/Compiler now carry the run's ScopeTable, since a node no longer carries its own scope -- see the parser PR for why it cannot. evaluate() with BOSL2 67.2ms -> 7.5ms (9x) BOSL2's 909-test suite 95.3s -> 39.1s (2.4x) A Closure also captures its defining scope now. It used to read it back off its FunctionLiteral node, which works only while the node holds one, and a closure can be called at generate time with no EvalContext left to look it up through. Watch out for one thing when adding an EvalContext constructor: childCtx() and callCtx() build a fresh context field by field, and the version of this that forgot to carry scopeTable made every node's scope read back as null. Name resolution then quietly fell back to the enclosing scope, so a module body calling a builtin of its own name found ITSELF -- BOSL2's `module _cube(...) cube(...);` wrapper recursed to the depth limit. All 1149 tests passed with that bug; BOSL2 caught it. There is now a test that checks each derived context directly, and it fails if the assignment is removed. 1153 C++ tests, 648 parser tests, 31 binding tests, BOSL2's 909. Co-Authored-By: Claude Opus 5 (1M context) --- bindings/module.cpp | 12 +- external/openscad_cpp_parser | 2 +- .../bytecode_compiler.hpp | 11 +- .../openscad_cpp_evaluator/eval_context.hpp | 17 ++- include/openscad_cpp_evaluator/eval_use.hpp | 9 ++ include/openscad_cpp_evaluator/evaluator.hpp | 11 ++ include/openscad_cpp_evaluator/value.hpp | 9 ++ pyproject.toml | 2 +- src/builtins/topology.cpp | 2 +- src/bytecode_compiler.cpp | 53 ++++--- src/bytecode_vm.cpp | 14 +- src/csg_resolve.cpp | 6 + src/eval_context.cpp | 17 ++- src/eval_use.cpp | 26 +++- src/expr_eval.cpp | 2 +- src/stmt_eval.cpp | 2 +- src/user_calls.cpp | 24 ++-- tests/CMakeLists.txt | 1 + tests/test_include_cache.cpp | 132 ++++++++++++++++++ 19 files changed, 294 insertions(+), 58 deletions(-) create mode 100644 tests/test_include_cache.cpp diff --git a/bindings/module.cpp b/bindings/module.cpp index 06bd71e..6839b77 100644 --- a/bindings/module.cpp +++ b/bindings/module.cpp @@ -388,8 +388,8 @@ nb::object evaluate(const std::string& path, nb::dict viewportParams, nb::gil_scoped_release rel; auto logFn = [&echoes](const std::string& m) { echoes.push_back(m); }; try { - std::vector> ast = oscad::getASTFromFile(path); - oscadeval::ResolvedUseScopes used = oscadeval::resolveUseScopes(ast, path, logFn); + oscad::ParsedProgram program = oscad::getProgramFromFile(path); + oscadeval::ResolvedUseScopes used = oscadeval::resolveUseScopes(program.nodes, path, logFn); oscadeval::Evaluator ev(logFn, nullptr, manifoldCache, oscadeval::DebugHooks{}, profile); oscadeval::EvalContext ctx = oscadeval::EvalContext::makeRoot(used.rootScope.get()); bodies = oscadeval::toRenderableBodies(ev.evaluate(used.processedNodes, ctx, vp, generate)); @@ -693,8 +693,8 @@ nb::object debugEvaluate(const std::string& path, nb::dict viewportParams, nb::c }; } try { - std::vector> ast = oscad::getASTFromFile(path); - oscadeval::ResolvedUseScopes used = oscadeval::resolveUseScopes(ast, path, echoCpp); + oscad::ParsedProgram program = oscad::getProgramFromFile(path); + oscadeval::ResolvedUseScopes used = oscadeval::resolveUseScopes(program.nodes, path, echoCpp); oscadeval::Evaluator ev(echoCpp, nullptr, manifoldCache, hooks, false); evPtr = &ev; if (fastContinueSignal) ev.setFastContinueInterruptFlag(fastContinueSignal->flag()); @@ -729,8 +729,8 @@ nb::list parseDecls(const std::string& path) { std::vector decls; { nb::gil_scoped_release rel; - std::vector> ast = oscad::getASTFromFile(path); - oscadeval::ResolvedUseScopes used = oscadeval::resolveUseScopes(ast, path, [](const std::string&) {}); + oscad::ParsedProgram program = oscad::getProgramFromFile(path); + oscadeval::ResolvedUseScopes used = oscadeval::resolveUseScopes(program.nodes, path, [](const std::string&) {}); for (const oscad::ASTNode* n : used.processedNodes) { const oscad::Position& p = n->position(); const char* ns = nullptr; diff --git a/external/openscad_cpp_parser b/external/openscad_cpp_parser index ade1d62..e4a75fc 160000 --- a/external/openscad_cpp_parser +++ b/external/openscad_cpp_parser @@ -1 +1 @@ -Subproject commit ade1d62073d3f47544bc562790a94310b549b7ab +Subproject commit e4a75fc48c9ca7cecd27edad4939aa26f7492480 diff --git a/include/openscad_cpp_evaluator/bytecode_compiler.hpp b/include/openscad_cpp_evaluator/bytecode_compiler.hpp index fd805fa..89ce9e0 100644 --- a/include/openscad_cpp_evaluator/bytecode_compiler.hpp +++ b/include/openscad_cpp_evaluator/bytecode_compiler.hpp @@ -20,7 +20,8 @@ namespace oscadeval { // yet -- the caller falls back to the ordinary AST interpreter for that // whole function, unconditionally correct since nothing about this // function's behavior changes based on whether it happened to compile. -std::optional tryCompileFunction(const oscad::FunctionDeclaration& decl); +std::optional tryCompileFunction(const oscad::FunctionDeclaration& decl, + const oscad::ScopeTable* scopeTable); // Compiles a bare STATEMENT-context expression -- an assignment's RHS, an // if/for condition, a module-call or echo()/assert() argument -- rather @@ -48,7 +49,8 @@ std::optional tryCompileFunction(const oscad::FunctionDeclaration // always set by buildScope() regardless of node kind) -- needed to resolve // any callee inside it statically, the same way a function body's call // sites are. -std::optional tryCompileStatementExpr(const oscad::Expression& expr, const oscad::Scope* scope); +std::optional tryCompileStatementExpr(const oscad::Expression& expr, const oscad::Scope* scope, + const oscad::ScopeTable* scopeTable); // Compiles a run of SIBLING assignment statements sharing one scope -- // exactly Evaluator::evalChildren's own `assignments` sub-list (stmt_eval. @@ -82,6 +84,7 @@ std::optional tryCompileStatementExpr(const oscad::Expression& ex // contained by, so it would leak into the caller's ctx.dyn permanently // instead of just for that one assignment's own RHS. std::optional tryCompileAssignmentBlock(const std::vector& assigns, + const oscad::ScopeTable* scopeTable, const oscad::Scope* scope); // Attempts to compile `decl`'s parameter defaults + STATEMENT-list body @@ -97,7 +100,8 @@ std::optional tryCompileAssignmentBlock(const std::vector tryCompileModuleBody(const oscad::ModuleDeclaration& decl); +std::optional tryCompileModuleBody(const oscad::ModuleDeclaration& decl, + const oscad::ScopeTable* scopeTable); // Same compilation (assignment/if/for/resolved-module-call get real // bytecode, everything else a native passthrough), but for an ARBITRARY @@ -121,6 +125,7 @@ std::optional tryCompileModuleBody(const oscad::ModuleDeclaration // resolving a call site's callee -- callers pass `children.front()-> // scope()`, mirroring tryRunCompiledAssignmentBlock's own convention. std::optional tryCompileChildrenList(const std::vector& children, + const oscad::ScopeTable* scopeTable, const oscad::Scope* scope); } // namespace oscadeval diff --git a/include/openscad_cpp_evaluator/eval_context.hpp b/include/openscad_cpp_evaluator/eval_context.hpp index 6528a8d..49e5063 100644 --- a/include/openscad_cpp_evaluator/eval_context.hpp +++ b/include/openscad_cpp_evaluator/eval_context.hpp @@ -6,6 +6,7 @@ #include "openscad_cpp_parser/ast/ast_node.hpp" #include "openscad_cpp_parser/position.hpp" #include "openscad_cpp_parser/scope.hpp" +#include "openscad_cpp_parser/scope_table.hpp" #include #include @@ -74,11 +75,25 @@ struct EvalContext { // one that sees it. bool viaChildren = false; + // Where every node's lexical Scope lives, since it cannot live in the + // node itself -- one parsed tree is shared by every script that + // includes it, and `include` puts those nodes in the INCLUDER's scope. + // Owned by the Evaluator for the length of a run; aliased, never + // copied, by every derived context. + const oscad::ScopeTable* scopeTable = nullptr; + + // This node's own lexical scope, or null if none was recorded. Reads + // like the old ASTNode::scope() it replaces. + const oscad::Scope* scopeOf(const oscad::ASTNode& node) const { + return scopeTable ? scopeTable->get(node) : nullptr; + } + // The one genuinely fresh construction: seeds `dyn` with OpenSCAD's // built-in $-variable defaults ($fn=0, $fa=12, $fs=2, $t=0, // $parent_modules=0). Every other EvalContext in a run is derived // from this one via the methods below. - static EvalContext makeRoot(const oscad::Scope* rootScope); + static EvalContext makeRoot(const oscad::Scope* rootScope, + const oscad::ScopeTable* scopeTable = nullptr); // Mirrors _eval_children's direct EvalContext(...) construction: swaps // only `scope` (to a sibling statement's own lexical scope from diff --git a/include/openscad_cpp_evaluator/eval_use.hpp b/include/openscad_cpp_evaluator/eval_use.hpp index 7737ddd..9eb46d5 100644 --- a/include/openscad_cpp_evaluator/eval_use.hpp +++ b/include/openscad_cpp_evaluator/eval_use.hpp @@ -29,6 +29,8 @@ struct ResolvedUseScopes { // these (not anything currentFile itself pulled in via `use`; "nested // use has no effect on the base file's environment"). std::vector ownNodesFiltered; + // Owns the ScopeTable holding every node's lexical scope -- see + // oscad::ScopeTable for why that cannot live in the nodes themselves. std::unique_ptr rootScope; }; @@ -48,4 +50,11 @@ struct ResolvedUseScopes { ResolvedUseScopes resolveUseScopes(const std::vector>& ownNodes, const std::string& currentFile, const std::function& logFn); +// Same, for a statement list that is BORROWED rather than owned -- what +// oscad::ParsedProgram hands back when includes come from the shared AST +// cache. The caller must keep that ParsedProgram alive alongside the +// result, exactly as it must keep an owned AST alive. +ResolvedUseScopes resolveUseScopes(const std::vector& ownNodes, const std::string& currentFile, + const std::function& logFn); + } // namespace oscadeval diff --git a/include/openscad_cpp_evaluator/evaluator.hpp b/include/openscad_cpp_evaluator/evaluator.hpp index 6c434b2..21cabf3 100644 --- a/include/openscad_cpp_evaluator/evaluator.hpp +++ b/include/openscad_cpp_evaluator/evaluator.hpp @@ -132,6 +132,16 @@ class Evaluator { // fallback. Value evalExprMaybeCompiled(const oscad::Expression& node, EvalContext& ctx); + // The run's ScopeTable, latched from the EvalContext handed to + // evaluate()/resolveTree(). A handful of chunk-compile helpers below + // need a node's scope but are reached from the VM without an + // EvalContext in hand; they read it here instead of every caller + // growing a parameter. Null outside a run, which reads the same as the + // unset ASTNode::scope() this replaced. + const oscad::Scope* scopeOfNode(const oscad::ASTNode& node) const { + return scopeTable_ ? scopeTable_->get(node) : nullptr; + } + // Evaluates a block's statements in OpenSCAD's assignment-before- // geometry order (all Assignment nodes first, then everything else, // each group preserving source order), each against its own lexical @@ -1191,6 +1201,7 @@ class Evaluator { // this exists. Not a reentrancy guard (resolveTreeImpl is never called // while another one is already active), just an on/off switch for // whether evalExprMaybeCompiled's cache is safe to touch right now. + const oscad::ScopeTable* scopeTable_ = nullptr; bool inResolvePass_ = false; // True only while evalRenderExpr (or the VM's Kind::Measure bracket) is diff --git a/include/openscad_cpp_evaluator/value.hpp b/include/openscad_cpp_evaluator/value.hpp index c5efcae..8668a3f 100644 --- a/include/openscad_cpp_evaluator/value.hpp +++ b/include/openscad_cpp_evaluator/value.hpp @@ -12,6 +12,7 @@ namespace oscad { class FunctionLiteral; +class Scope; } // namespace oscad namespace oscadeval { @@ -75,6 +76,14 @@ using Value = std::variant capturedLet; + // The literal's own lexical scope, captured when the closure is made. + // A node no longer carries its scope (one parsed tree is shared + // between evaluations -- see oscad::ScopeTable), and a closure can be + // called from generate time, where there is no EvalContext left to + // look it up through. Not part of identity: operator== still compares + // `node`, since the same literal always resolves to the same scope + // within one run. + const oscad::Scope* scope = nullptr; friend bool operator==(const Closure& a, const Closure& b) { return a.node == b.node; } }; diff --git a/pyproject.toml b/pyproject.toml index ccad006..115970f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build" [project] name = "openscad_cpp_evaluator" -version = "1.3.1" +version = "1.4.0" description = "C++ OpenSCAD evaluator with Python bindings" readme = "README.md" requires-python = ">=3.12" diff --git a/src/builtins/topology.cpp b/src/builtins/topology.cpp index 4732d37..ef89768 100644 --- a/src/builtins/topology.cpp +++ b/src/builtins/topology.cpp @@ -873,7 +873,7 @@ std::vector generateLevelSet(Evaluator& ev, const CSGParams& params std::optional fnCtx; std::vector fnParams2; if (fieldFn) { - fnCtx = EvalContext::makeRoot((*fieldFn)->node->scope()); + fnCtx = EvalContext::makeRoot((*fieldFn)->scope); for (const auto& prm : (*fieldFn)->node->parameters) fnParams2.push_back(prm->name->name); } diff --git a/src/bytecode_compiler.cpp b/src/bytecode_compiler.cpp index 6940898..26690be 100644 --- a/src/bytecode_compiler.cpp +++ b/src/bytecode_compiler.cpp @@ -216,7 +216,7 @@ class Compiler; // Returns false (chunk left partially populated, discarded by the caller) // for any NotCompilable thrown while compiling. bool compileFunctionLike(CompiledChunk& chunk, const oscad::Scope* staticScope, const oscad::ASTNode* selfDecl, - std::vector enclosing, + const oscad::ScopeTable* scopeTable, std::vector enclosing, const std::vector>& params, const oscad::Expression& bodyExpr); @@ -238,8 +238,16 @@ class Compiler { // when compiling a FunctionLiteral nested inside another compile -- // see the FunctionLiteral case in compileExpr). Compiler(CompiledChunk& chunk, const oscad::Scope* scope, const oscad::ASTNode* selfDecl, - std::vector enclosing) - : chunk_(chunk), scope_(scope), selfDecl_(selfDecl), enclosing_(std::move(enclosing)) {} + const oscad::ScopeTable* scopeTable, std::vector enclosing) + : chunk_(chunk), scope_(scope), selfDecl_(selfDecl), scopeTable_(scopeTable), + enclosing_(std::move(enclosing)) {} + + // A node's own lexical scope, or null. Replaces ASTNode::scope(), + // which is gone: one parsed tree is shared between evaluations, so a + // node's scope lives in the run's ScopeTable, not the node. + const oscad::Scope* scopeOf(const oscad::ASTNode& node) const { + return scopeTable_ ? scopeTable_->get(node) : nullptr; + } int nextSlot() const { return nextSlot_; } int nextIterList() const { return nextIterList_; } @@ -408,7 +416,7 @@ class Compiler { std::vector childEnclosing = enclosing_; childEnclosing.push_back({selfDecl_, &scope}); CompiledChunk literalChunk; - if (!compileFunctionLike(literalChunk, n.scope(), &n, std::move(childEnclosing), n.parameters, + if (!compileFunctionLike(literalChunk, scopeOf(n), &n, scopeTable_, std::move(childEnclosing), n.parameters, *n.body)) { throw NotCompilable{}; } @@ -1461,11 +1469,12 @@ class Compiler { struct ScopeGuard { const oscad::Scope*& scope; const oscad::Scope* saved; - ScopeGuard(const oscad::Scope*& s, const oscad::ASTNode& node) : scope(s), saved(s) { - if (const oscad::Scope* stmtScope = node.scope()) scope = stmtScope; + ScopeGuard(const oscad::Scope*& s, const oscad::ASTNode& node, const oscad::ScopeTable* table) + : scope(s), saved(s) { + if (const oscad::Scope* stmtScope = table ? table->get(node) : nullptr) scope = stmtScope; } ~ScopeGuard() { scope = saved; } - } scopeGuard(scope_, stmt); + } scopeGuard(scope_, stmt, scopeTable_); switch (stmt.kind()) { // Pure declarations, no-ops at statement-eval time (already // hoisted into scope by buildScopes()) -- matches evalStatement's @@ -1612,7 +1621,7 @@ class Compiler { } case NodeKind::ModularCall: { auto& call = static_cast(stmt); - const oscad::Scope* lookupScope = stmt.scope() ? stmt.scope() : scope_; + const oscad::Scope* lookupScope = scopeOf(stmt) ? scopeOf(stmt) : scope_; const oscad::ASTNode* resolved = lookupScope ? lookupScope->lookupModule(call.name->name) : nullptr; if (resolved && resolved->kind() == NodeKind::ModuleDeclaration) { CompiledChunk::ModuleCallSite site; @@ -1842,17 +1851,18 @@ class Compiler { CompiledChunk& chunk_; const oscad::Scope* scope_; const oscad::ASTNode* selfDecl_; + const oscad::ScopeTable* scopeTable_ = nullptr; std::vector enclosing_; int nextSlot_ = 0; int nextIterList_ = 0; }; bool compileFunctionLike(CompiledChunk& chunk, const oscad::Scope* staticScope, const oscad::ASTNode* selfDecl, - std::vector enclosing, + const oscad::ScopeTable* scopeTable, std::vector enclosing, const std::vector>& params, const oscad::Expression& bodyExpr) { chunk.selfDecl = selfDecl; - Compiler compiler(chunk, staticScope, selfDecl, std::move(enclosing)); + Compiler compiler(chunk, staticScope, selfDecl, scopeTable, std::move(enclosing)); CompileScope bodyScope; bodyScope.push(); for (const auto& p : params) { @@ -1888,16 +1898,20 @@ bool compileFunctionLike(CompiledChunk& chunk, const oscad::Scope* staticScope, } // namespace -std::optional tryCompileFunction(const oscad::FunctionDeclaration& decl) { +std::optional tryCompileFunction(const oscad::FunctionDeclaration& decl, + const oscad::ScopeTable* scopeTable) { CompiledChunk chunk; - if (!compileFunctionLike(chunk, decl.scope(), &decl, {}, decl.parameters, *decl.expr)) return std::nullopt; + if (!compileFunctionLike(chunk, scopeTable ? scopeTable->get(decl) : nullptr, &decl, scopeTable, {}, + decl.parameters, *decl.expr)) + return std::nullopt; return chunk; } -std::optional tryCompileStatementExpr(const oscad::Expression& expr, const oscad::Scope* scope) { +std::optional tryCompileStatementExpr(const oscad::Expression& expr, const oscad::Scope* scope, + const oscad::ScopeTable* scopeTable) { static const std::vector> kNoParams; CompiledChunk chunk; - if (!compileFunctionLike(chunk, scope, nullptr, {}, kNoParams, expr)) return std::nullopt; + if (!compileFunctionLike(chunk, scope, nullptr, scopeTable, {}, kNoParams, expr)) return std::nullopt; // See this function's own doc comment (bytecode_compiler.hpp) for why a // captures-having nested closure can't be supported by this bare // wrapper -- selfDecl is nullptr and enclosing is empty above, so its @@ -1910,6 +1924,7 @@ std::optional tryCompileStatementExpr(const oscad::Expression& ex } std::optional tryCompileAssignmentBlock(const std::vector& assigns, + const oscad::ScopeTable* scopeTable, const oscad::Scope* scope) { // Reassignment-warning fidelity (see this function's own doc comment, // bytecode_compiler.hpp) -- cheap, one-time scan before touching the @@ -1926,7 +1941,7 @@ std::optional tryCompileAssignmentBlock(const std::vector tryCompileAssignmentBlock(const std::vector tryCompileModuleBody(const oscad::ModuleDeclaration& decl) { +std::optional tryCompileModuleBody(const oscad::ModuleDeclaration& decl, + const oscad::ScopeTable* scopeTable) { CompiledChunk chunk; chunk.isModule = true; chunk.selfDecl = &decl; @@ -1978,7 +1994,7 @@ std::optional tryCompileModuleBody(const oscad::ModuleDeclaration // compile path, just scoped to that one statement's own sub- // expression -- see this file's own module-chunk doc comment // (bytecode.hpp) for the full reasoning. - Compiler compiler(chunk, decl.scope(), nullptr, {}); + Compiler compiler(chunk, scopeTable ? scopeTable->get(decl) : nullptr, nullptr, scopeTable, {}); try { compiler.compileStatementList(decl.children, chunk.bodyCode); } catch (const NotCompilable&) { @@ -1990,6 +2006,7 @@ std::optional tryCompileModuleBody(const oscad::ModuleDeclaration } std::optional tryCompileChildrenList(const std::vector& children, + const oscad::ScopeTable* scopeTable, const oscad::Scope* scope) { CompiledChunk chunk; // Same completion semantics as a module chunk (no return value, its @@ -2001,7 +2018,7 @@ std::optional tryCompileChildrenList(const std::vectorfind(cap.name); capturedTrail->set(cap.name, v ? *v : Value{}); } - auto closure = std::make_shared(Closure{site.node, capturedTrail}); + auto closure = std::make_shared(Closure{site.node, capturedTrail, ctx.scopeOf(*site.node)}); for (const std::string* selfName : selfNames) { capturedTrail->set(*selfName, Value{closure}); } @@ -810,7 +810,7 @@ Value driveVm(Evaluator& ev, size_t floor) { BoundArgs bound = buildBoundArgs(ev, site, args, argCount, site.decl->parameters); const CompiledChunk* calleeChunk = ev.useBytecodeVm() ? ev.lookupOrCompileChunk(*site.decl) : nullptr; if (calleeChunk) { - const oscad::Scope* fnScope = site.decl->scope() ? site.decl->scope() : ctx.scope; + const oscad::Scope* fnScope = ctx.scopeOf(*site.decl) ? ctx.scopeOf(*site.decl) : ctx.scope; pushBracketedCallFrame(ev, *calleeChunk, *site.decl, *site.decl->expr, site.calleeName, std::move(bound), ctx, fnScope, nullptr, &site.callNode->position()); // f.pc deliberately NOT advanced -- resumes when the @@ -841,7 +841,7 @@ Value driveVm(Evaluator& ev, size_t floor) { BoundArgs bound = buildBoundArgs(ev, site, args, argCount, funcNode.parameters); const CompiledChunk* calleeChunk = ev.useBytecodeVm() ? ev.lookupCompiledLiteralChunk(funcNode) : nullptr; if (calleeChunk) { - const oscad::Scope* fnScope = funcNode.scope() ? funcNode.scope() : ctx.scope; + const oscad::Scope* fnScope = ctx.scopeOf(funcNode) ? ctx.scopeOf(funcNode) : ctx.scope; pushBracketedCallFrame(ev, *calleeChunk, funcNode, *funcNode.body, "", std::move(bound), ctx, fnScope, capturedLetTrail(closure), ins.pos); } else { @@ -906,7 +906,7 @@ Value driveVm(Evaluator& ev, size_t floor) { const CompiledChunk* fallbackChunk = ev.useBytecodeVm() ? ev.lookupOrCompileChunk(*site.decl) : nullptr; if (fallbackChunk) { - const oscad::Scope* fnScope = site.decl->scope() ? site.decl->scope() : ctx.scope; + const oscad::Scope* fnScope = ctx.scopeOf(*site.decl) ? ctx.scopeOf(*site.decl) : ctx.scope; pushBracketedCallFrame(ev, *fallbackChunk, *site.decl, *site.decl->expr, site.calleeName, std::move(bound), ctx, fnScope, nullptr, &site.callNode->position()); } else { @@ -956,7 +956,7 @@ Value driveVm(Evaluator& ev, size_t floor) { const CompiledChunk* fallbackChunk = ev.useBytecodeVm() ? ev.lookupCompiledLiteralChunk(funcNode) : nullptr; if (fallbackChunk) { - const oscad::Scope* fnScope = funcNode.scope() ? funcNode.scope() : ctx.scope; + const oscad::Scope* fnScope = ctx.scopeOf(funcNode) ? ctx.scopeOf(funcNode) : ctx.scope; pushBracketedCallFrame(ev, *fallbackChunk, funcNode, *funcNode.body, "", std::move(bound), ctx, fnScope, capturedLetTrail(closure), ins.pos); } else { @@ -1040,7 +1040,7 @@ Value driveVm(Evaluator& ev, size_t floor) { // unobservable). const auto* callNode = static_cast(f.chunk->nativeStatements[static_cast(ins.a)]); - EvalContext scopedCtx = ctx.withScope(callNode->scope() ? callNode->scope() : ctx.scope); + EvalContext scopedCtx = ctx.withScope(ctx.scopeOf(*callNode) ? ctx.scopeOf(*callNode) : ctx.scope); ev.checkDebug(*callNode, scopedCtx); // Same order as evalModularCall's own (csg_resolve.cpp): // warn, then resolve. Without this a children() typo @@ -1398,7 +1398,7 @@ Value driveVm(Evaluator& ev, size_t floor) { } case Op::NativeStatement: { const oscad::ASTNode* stmt = f.chunk->nativeStatements[static_cast(ins.a)]; - EvalContext childCtx = ctx.withScope(stmt->scope() ? stmt->scope() : ctx.scope); + EvalContext childCtx = ctx.withScope(ctx.scopeOf(*stmt) ? ctx.scopeOf(*stmt) : ctx.scope); // Mirrors evalChildren's own per-statement loop exactly // -- no ModularLet exclusion needed here (unlike that // loop's own): ModularLet has its own compiled form now diff --git a/src/csg_resolve.cpp b/src/csg_resolve.cpp index d456543..6038ee1 100644 --- a/src/csg_resolve.cpp +++ b/src/csg_resolve.cpp @@ -205,6 +205,9 @@ void Evaluator::evalIntersectionForNode(const oscad::ModularIntersectionFor& nod template std::vector> Evaluator::resolveTreeImpl(const NodeList& nodes, EvalContext& ctx) { + // Latch the run's ScopeTable for the helpers the VM reaches without an + // EvalContext -- see Evaluator::scopeOfNode. + scopeTable_ = ctx.scopeTable; idToNode.clear(); idToColor.clear(); syntheticNodes_.clear(); @@ -246,6 +249,9 @@ template std::vector Evaluator::evaluateImpl(const NodeList& nodes, EvalContext& ctx, const std::unordered_map& viewportParams, bool generate) { + // Latch the run's ScopeTable for the helpers the VM reaches without an + // EvalContext -- see Evaluator::scopeOfNode. + scopeTable_ = ctx.scopeTable; // Seeds ctx.dyn directly, deliberately not touching ctx.dynExplicit -- // see this method's own doc comment in evaluator.hpp for why that // distinction matters to a caller. diff --git a/src/eval_context.cpp b/src/eval_context.cpp index 436a25b..09245a9 100644 --- a/src/eval_context.cpp +++ b/src/eval_context.cpp @@ -2,9 +2,12 @@ namespace oscadeval { -EvalContext EvalContext::makeRoot(const oscad::Scope* rootScope) { +EvalContext EvalContext::makeRoot(const oscad::Scope* rootScope, const oscad::ScopeTable* scopeTable) { + // Default: the table buildScopes() attached to this scope tree's root. + if (!scopeTable && rootScope) scopeTable = rootScope->rootTable(); EvalContext ctx; ctx.scope = rootScope; + ctx.scopeTable = scopeTable; // dyn/dynExplicit share one underlying trail -- see scope_trail.hpp's // own doc comment on DynValueView/DynExplicitView. auto dynTrail = IndexedTrailView::makeRoot(std::make_shared()); @@ -66,6 +69,10 @@ EvalContext EvalContext::childCtx(const oscad::Scope* newScope, std::optional newChildrenNodes, const EvalContext* newChildrenCallerCtx) const { EvalContext result; + // Constant for the whole run; a derived context must never lose it, or + // a node's scope reads back as null and name resolution silently falls + // back to the enclosing scope. + result.scopeTable = scopeTable; result.scope = newScope ? newScope : scope; auto newDynTrail = dyn.trail()->openChild(/*isolate=*/false); result.dyn = DynValueView(newDynTrail); @@ -83,6 +90,10 @@ EvalContext EvalContext::callCtx(const oscad::Scope* newScope, std::optional newChildrenNodes, const EvalContext* newChildrenCallerCtx) const { EvalContext result; + // Constant for the whole run; a derived context must never lose it, or + // a node's scope reads back as null and name resolution silently falls + // back to the enclosing scope. + result.scopeTable = scopeTable; result.scope = newScope ? newScope : scope; auto newDynTrail = dyn.trail()->openChild(/*isolate=*/false); // stays dynamically scoped through result.dyn = DynValueView(newDynTrail); @@ -101,6 +112,10 @@ EvalContext EvalContext::callCtxFromCapturedLet(const std::shared_ptr newChildrenNodes, const EvalContext* newChildrenCallerCtx) const { EvalContext result; + // Constant for the whole run; a derived context must never lose it, or + // a node's scope reads back as null and name resolution silently falls + // back to the enclosing scope. + result.scopeTable = scopeTable; result.scope = newScope ? newScope : scope; auto newDynTrail = dyn.trail()->openChild(/*isolate=*/false); // stays dynamically scoped through the CALL SITE result.dyn = DynValueView(newDynTrail); diff --git a/src/eval_use.cpp b/src/eval_use.cpp index 48136f7..781def3 100644 --- a/src/eval_use.cpp +++ b/src/eval_use.cpp @@ -8,6 +8,17 @@ namespace oscadeval { ResolvedUseScopes resolveUseScopes(const std::vector>& ownNodes, const std::string& currentFile, const std::function& logFn) { + // Borrow, then share the one implementation: nothing here needs + // ownership, and the cached-include path (ParsedProgram) only ever has + // borrowed nodes to offer. + std::vector borrowed; + borrowed.reserve(ownNodes.size()); + for (const auto& n : ownNodes) borrowed.push_back(n.get()); + return resolveUseScopes(borrowed, currentFile, logFn); +} + +ResolvedUseScopes resolveUseScopes(const std::vector& ownNodes, + const std::string& currentFile, const std::function& logFn) { ResolvedUseScopes result; std::vector injected; @@ -16,7 +27,7 @@ ResolvedUseScopes resolveUseScopes(const std::vector, oscad::Scope*>> reanchor; - for (const auto& nodePtr : ownNodes) { + for (const oscad::ASTNode* nodePtr : ownNodes) { if (nodePtr->kind() != oscad::NodeKind::UseStatement) continue; const auto& useNode = static_cast(*nodePtr); @@ -64,8 +75,8 @@ ResolvedUseScopes resolveUseScopes(const std::vectorkind() != oscad::NodeKind::UseStatement) result.ownNodesFiltered.push_back(nodePtr.get()); + for (const oscad::ASTNode* nodePtr : ownNodes) { + if (nodePtr->kind() != oscad::NodeKind::UseStatement) result.ownNodesFiltered.push_back(nodePtr); } result.processedNodes = injected; @@ -83,8 +94,13 @@ ResolvedUseScopes resolveUseScopes(const std::vector(n)); result.rootScope = oscad::buildScopes(mutableProcessed); - for (const auto& [libInjected, libRootScope] : reanchor) { - for (const oscad::ASTNode* n : libInjected) const_cast(n)->buildScope(*libRootScope); + { + // Re-anchoring writes scopes too, into the same table buildScopes() + // just filled -- which the root scope now owns. + oscad::ScopeTableScope recording(*const_cast(result.rootScope->table())); + for (const auto& [libInjected, libRootScope] : reanchor) { + for (const oscad::ASTNode* n : libInjected) const_cast(n)->buildScope(*libRootScope); + } } return result; diff --git a/src/expr_eval.cpp b/src/expr_eval.cpp index f715dfa..6195b68 100644 --- a/src/expr_eval.cpp +++ b/src/expr_eval.cpp @@ -456,7 +456,7 @@ Value Evaluator::evalExpr(const oscad::Expression& node, EvalContext& ctx) { // time, matching real OpenSCAD), and the lexical scope is // static AST data already reachable via the node itself. return Value{std::make_shared( - Closure{static_cast(&node), ctx.let_})}; + Closure{static_cast(&node), ctx.let_, ctx.scopeOf(node)})}; case NodeKind::PrimaryCall: return evalFunctionCall(static_cast(node), ctx); case NodeKind::LetOp: diff --git a/src/stmt_eval.cpp b/src/stmt_eval.cpp index 087397d..230c5ad 100644 --- a/src/stmt_eval.cpp +++ b/src/stmt_eval.cpp @@ -291,7 +291,7 @@ void Evaluator::evalChildren(const std::vector& children, auto runAll = [&](const std::vector& nodes) { for (const oscad::ASTNode* child : nodes) { - const oscad::Scope* childScope = child->scope() ? child->scope() : ctx.scope; + const oscad::Scope* childScope = ctx.scopeOf(*child) ? ctx.scopeOf(*child) : ctx.scope; EvalContext childCtx = ctx.withScope(childScope); // Safe despite childCtx's own scope ending at this iteration's // close: lastCtx_ is only ever read (by error()) synchronously diff --git a/src/user_calls.cpp b/src/user_calls.cpp index 918d10b..2920910 100644 --- a/src/user_calls.cpp +++ b/src/user_calls.cpp @@ -11,7 +11,7 @@ namespace oscadeval { const CompiledChunk* Evaluator::lookupOrCompileChunk(const oscad::FunctionDeclaration& decl) { auto it = chunkCache_.find(&decl); if (it == chunkCache_.end()) { - it = chunkCache_.emplace(&decl, tryCompileFunction(decl)).first; + it = chunkCache_.emplace(&decl, tryCompileFunction(decl, scopeTable_)).first; if (it->second) flattenNestedLiterals(*it->second); } if (!it->second) return nullptr; @@ -33,7 +33,7 @@ const CompiledChunk* Evaluator::lookupCompiledLiteralChunk(const oscad::Function const CompiledChunk* Evaluator::lookupOrCompileModuleChunk(const oscad::ModuleDeclaration& decl) { auto it = moduleChunkCache_.find(&decl); if (it == moduleChunkCache_.end()) { - it = moduleChunkCache_.emplace(&decl, tryCompileModuleBody(decl)).first; + it = moduleChunkCache_.emplace(&decl, tryCompileModuleBody(decl, scopeTable_)).first; if (it->second) flattenNestedLiterals(*it->second); } if (!it->second) return nullptr; @@ -52,7 +52,7 @@ Value Evaluator::evalExprMaybeCompiled(const oscad::Expression& node, EvalContex if (!useBytecodeVm() || !inResolvePass_) return evalExpr(node, ctx); auto it = stmtExprChunkCache_.find(&node); if (it == stmtExprChunkCache_.end()) { - it = stmtExprChunkCache_.emplace(&node, tryCompileStatementExpr(node, node.scope())).first; + it = stmtExprChunkCache_.emplace(&node, tryCompileStatementExpr(node, scopeOfNode(node), scopeTable_)).first; // A zero-capture closure literal (e.g. `x = function(y) y + 1;`) // still reaches chunk.nestedLiterals even though it never touches // closureSites (see tryCompileStatementExpr's own doc comment: only @@ -74,7 +74,7 @@ bool Evaluator::tryRunCompiledAssignmentBlock(const std::vector assigns; assigns.reserve(assignments.size()); for (const oscad::ASTNode* n : assignments) assigns.push_back(static_cast(n)); - it = assignBlockChunkCache_.emplace(first, tryCompileAssignmentBlock(assigns, first->scope())).first; + it = assignBlockChunkCache_.emplace(first, tryCompileAssignmentBlock(assigns, scopeTable_, scopeOfNode(*first))).first; if (it->second) flattenNestedLiterals(*it->second); } if (!it->second || !chunkEligibleNow(*it->second)) return false; @@ -102,7 +102,7 @@ const CompiledChunk* Evaluator::lookupOrCompileChildrenListChunk(const std::vect const auto key = std::make_pair(first, children.size()); auto it = childrenListChunkCache_.find(key); if (it == childrenListChunkCache_.end()) { - it = childrenListChunkCache_.emplace(key, tryCompileChildrenList(children, first->scope())).first; + it = childrenListChunkCache_.emplace(key, tryCompileChildrenList(children, scopeTable_, scopeOfNode(*first))).first; if (it->second) flattenNestedLiterals(*it->second); } if (!it->second || !chunkEligibleNow(*it->second)) return nullptr; @@ -277,7 +277,7 @@ std::optional Evaluator::isolatedCallCtxFor(const oscad::ASTNode& d // scope to walk outward from -- the caller's scope at the call site // has no relation to it at all. bool usedChildCtx = false; - const oscad::Scope* declScope = declNode.scope() ? declNode.scope() : ctx.scope; + const oscad::Scope* declScope = ctx.scopeOf(declNode) ? ctx.scopeOf(declNode) : ctx.scope; EvalContext result = callCtxFor(declNode, ctx, declScope, nullptr, nullptr, &usedChildCtx, capturedLet); if (usedChildCtx) return std::nullopt; return result; @@ -337,7 +337,7 @@ void Evaluator::bindCallArgsInto(const std::vector Evaluator::tryTailStepFor( bool hasCompiledChunk, const std::vector>& arguments, EvalContext& ctx, const oscad::Position& callPos, const std::shared_ptr>& capturedLet) { if (hasCompiledChunk) return std::nullopt; - const oscad::Scope* fnScope = declNode.scope() ? declNode.scope() : ctx.scope; + const oscad::Scope* fnScope = ctx.scopeOf(declNode) ? ctx.scopeOf(declNode) : ctx.scope; bool usedChildCtx = false; EvalContext childCtx = callCtxFor(declNode, ctx, fnScope, nullptr, nullptr, &usedChildCtx, capturedLet); if (usedChildCtx) return std::nullopt; @@ -769,7 +769,7 @@ void Evaluator::exitUserCallException(const UserCallHandle& handle) { Value Evaluator::evalUserFunction(const std::string& name, const oscad::FunctionDeclaration& decl, const std::vector>& arguments, EvalContext& ctx, const oscad::ASTNode* callNode) { - const oscad::Scope* fnScope = decl.scope() ? decl.scope() : ctx.scope; + const oscad::Scope* fnScope = ctx.scopeOf(decl) ? ctx.scopeOf(decl) : ctx.scope; const int callerFrameIdx = callStack_.empty() ? -1 : static_cast(callStack_.size()) - 1; bool usedChildCtx = false; EvalContext childCtx = callCtxFor(decl, ctx, fnScope, nullptr, nullptr, &usedChildCtx); @@ -794,7 +794,7 @@ Value Evaluator::evalUserFunction(const std::string& name, const oscad::Function Value Evaluator::evalUserFunctionFromBound(const std::string& name, const oscad::FunctionDeclaration& decl, BoundArgs bound, EvalContext& ctx, const oscad::Position* callPos) { - const oscad::Scope* fnScope = decl.scope() ? decl.scope() : ctx.scope; + const oscad::Scope* fnScope = ctx.scopeOf(decl) ? ctx.scopeOf(decl) : ctx.scope; const int callerFrameIdx = callStack_.empty() ? -1 : static_cast(callStack_.size()) - 1; bool usedChildCtx = false; EvalContext childCtx = callCtxFor(decl, ctx, fnScope, nullptr, nullptr, &usedChildCtx); @@ -814,7 +814,7 @@ Value Evaluator::evalFunctionLiteral(const Closure& closure, const std::vector>& arguments, EvalContext& ctx, const oscad::ASTNode* callNode) { const oscad::FunctionLiteral& funcNode = *closure.node; - const oscad::Scope* fnScope = funcNode.scope() ? funcNode.scope() : ctx.scope; + const oscad::Scope* fnScope = ctx.scopeOf(funcNode) ? ctx.scopeOf(funcNode) : ctx.scope; const int callerFrameIdx = callStack_.empty() ? -1 : static_cast(callStack_.size()) - 1; bool usedChildCtx = false; EvalContext childCtx = @@ -850,7 +850,7 @@ Value Evaluator::evalFunctionLiteral(const Closure& closure, Value Evaluator::evalFunctionLiteralFromBound(const Closure& closure, BoundArgs bound, EvalContext& ctx, const oscad::Position* callPos) { const oscad::FunctionLiteral& funcNode = *closure.node; - const oscad::Scope* fnScope = funcNode.scope() ? funcNode.scope() : ctx.scope; + const oscad::Scope* fnScope = ctx.scopeOf(funcNode) ? ctx.scopeOf(funcNode) : ctx.scope; const int callerFrameIdx = callStack_.empty() ? -1 : static_cast(callStack_.size()) - 1; bool usedChildCtx = false; EvalContext childCtx = diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 353af09..52d8829 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -27,6 +27,7 @@ add_executable(oscad_eval_tests test_function_builtins.cpp test_import_export.cpp test_zip_stored.cpp + test_include_cache.cpp test_use.cpp test_extrude_roof.cpp test_surface.cpp diff --git a/tests/test_include_cache.cpp b/tests/test_include_cache.cpp new file mode 100644 index 0000000..ccd5c8c --- /dev/null +++ b/tests/test_include_cache.cpp @@ -0,0 +1,132 @@ +// The shared include cache (oscad::getProgramFromFile) and the ScopeTable +// that makes sharing possible (oscad::ScopeTable). +// +// A parsed tree used to be owned by whoever asked for it, and each node +// carried its own Scope*. Both had to change together: sharing one tree +// between evaluations is what makes a re-render cheap (parsing +// BOSL2/std.scad is ~55ms and ~82% of evaluating a small script against +// it), and a shared node cannot hold a scope, because `include` puts it in +// the INCLUDER's scope -- a different one per script. + +#include "openscad_cpp_evaluator/eval_use.hpp" +#include "openscad_cpp_evaluator/evaluator.hpp" + +#include "openscad_cpp_parser/api.hpp" + +#include +#include +#include + +using namespace oscadeval; + +namespace { + +std::filesystem::path writeFile(const std::string& name, const std::string& content) { + const auto p = std::filesystem::temp_directory_path() / ("oscad_inccache_" + name); + std::ofstream out(p); + out << content; + return p; +} + +std::vector echoesOf(const std::filesystem::path& path) { + std::vector echoes; + auto log = [&echoes](const std::string& m) { echoes.push_back(m); }; + oscad::ParsedProgram program = oscad::getProgramFromFile(path.string()); + ResolvedUseScopes used = resolveUseScopes(program.nodes, path.string(), log); + Evaluator ev(log); + EvalContext ctx = EvalContext::makeRoot(used.rootScope.get()); + ev.evaluate(used.processedNodes, ctx, {}, /*generate=*/false); + return echoes; +} + +} // namespace + +TEST(IncludeCache, SameFileIncludedTwiceContributesItsStatementsOnce) { + // A file is spliced in at most once per resolution. This is why only + // the per-FILE parses are cached and never a whole resolved program: + // caching `mid` already-resolved would splice `base` twice here. + writeFile("base.scad", "BASE = 1;\n"); + const auto mid = writeFile("mid.scad", "include \nMID = 2;\n"); + const auto top = writeFile("top.scad", + "include \n" + "include \n" + "echo(BASE + MID);\n"); + (void)mid; + oscad::ParsedProgram program = oscad::getProgramFromFile(top.string()); + int baseAssignments = 0; + for (const oscad::ASTNode* n : program.nodes) { + if (n->kind() == oscad::NodeKind::Assignment && + static_cast(*n).name->name == "BASE") + ++baseAssignments; + } + EXPECT_EQ(baseAssignments, 1); + EXPECT_EQ(echoesOf(top), std::vector{"ECHO: 3"}); +} + +TEST(IncludeCache, TwoProgramsShareTheSameParsedNodes) { + // The whole point: the second script must not re-parse the library. + writeFile("lib.scad", "LIB = 7;\nfunction libf(x) = x * 2;\n"); + const auto a = writeFile("a.scad", "include \necho(libf(LIB));\n"); + const auto b = writeFile("b.scad", "include \necho(libf(LIB) + 1);\n"); + + oscad::ParsedProgram pa = oscad::getProgramFromFile(a.string()); + oscad::ParsedProgram pb = oscad::getProgramFromFile(b.string()); + // Same node ADDRESSES, not merely equal trees. + EXPECT_EQ(pa.nodes.front(), pb.nodes.front()); + + // And both still evaluate correctly, each in its own scope. + EXPECT_EQ(echoesOf(a), std::vector{"ECHO: 14"}); + EXPECT_EQ(echoesOf(b), std::vector{"ECHO: 15"}); +} + +TEST(IncludeCache, EditingAnIncludedFileReplacesItsEntryRatherThanAddingOne) { + // Keyed on (path, mtime, size), so an edit invalidates on its own. + const auto lib = writeFile("edited.scad", "V = 1;\n"); + const auto top = writeFile("edtop.scad", "include \necho(V);\n"); + EXPECT_EQ(echoesOf(top), std::vector{"ECHO: 1"}); + const size_t cacheEntriesBefore = oscad::astCacheSize(); + + std::filesystem::last_write_time(lib, std::filesystem::last_write_time(lib) + std::chrono::seconds(2)); + { std::ofstream out(lib); out << "V = 99;\n"; } + std::filesystem::last_write_time(lib, std::filesystem::last_write_time(lib) + std::chrono::seconds(4)); + EXPECT_EQ(echoesOf(top), std::vector{"ECHO: 99"}); + + // The old version must not still be held: an editor re-renders on every + // save, so keying the cache by (path, stamp) instead of replacing in + // place would keep a full AST copy of every version ever saved. + oscad::ParsedProgram again = oscad::getProgramFromFile(top.string()); + const oscad::ASTNode* first = again.nodes.front(); + ASSERT_EQ(first->kind(), oscad::NodeKind::Assignment); + EXPECT_EQ(oscad::astCacheSize(), cacheEntriesBefore) << "editing a file must replace its entry, not add one"; +} + +TEST(IncludeCache, ADerivedContextKeepsTheScopeTable) { + // The bug this exists for: EvalContext::childCtx/callCtx build a FRESH + // context field by field, and one that forgot to carry scopeTable made + // every node's scope read back as null. Name resolution then fell back + // to the enclosing scope, so a module body calling a builtin of its own + // name found ITSELF -- BOSL2's `module _cube(...) cube(...);` wrapper + // recursed until the depth limit. Nothing else in the suite noticed. + const auto top = writeFile("derived.scad", + "module _mycube(s) cube(s);\n" + "module wrap(s) _mycube(s);\n" + "wrap(2);\n"); + std::vector logs; + auto log = [&logs](const std::string& m) { logs.push_back(m); }; + oscad::ParsedProgram program = oscad::getProgramFromFile(top.string()); + ResolvedUseScopes used = resolveUseScopes(program.nodes, top.string(), log); + Evaluator ev(log); + EvalContext ctx = EvalContext::makeRoot(used.rootScope.get()); + ASSERT_NE(ctx.scopeTable, nullptr); + + // Every derived shape must carry it -- checked directly, so a new + // constructor that forgets fails here rather than as a recursion + // blow-up in somebody's library. + EXPECT_EQ(ctx.withScope(ctx.scope).scopeTable, ctx.scopeTable); + EXPECT_EQ(ctx.childCtx().scopeTable, ctx.scopeTable); + EXPECT_EQ(ctx.callCtx().scopeTable, ctx.scopeTable); + EXPECT_EQ(ctx.letChildCtx().scopeTable, ctx.scopeTable); + + ev.evaluate(used.processedNodes, ctx, {}, /*generate=*/false); + for (const std::string& m : logs) EXPECT_EQ(m.find("Recursion"), std::string::npos) << m; +}