diff --git a/design-docs/dd-061-interpreter-performance-roadmap.md b/design-docs/dd-061-interpreter-performance-roadmap.md index f785128..cac49e3 100644 --- a/design-docs/dd-061-interpreter-performance-roadmap.md +++ b/design-docs/dd-061-interpreter-performance-roadmap.md @@ -326,31 +326,33 @@ Acceptance criteria: Candidate benchmark note: local 3s/3-run `wrk` pass at 16 connections/2 threads, comparing `origin/main` at `52554e9` to this PR's dev-release binary, showed `/template/layout` RPS `23807.29 -> 24701.15` (+3.8%) and `/template/rows` RPS `6053.89 -> 6420.77` (+6.1%), while non-template routes stayed within noise. -### PR 5: Direct native/global call fast path +### PR 5: Targeted native/global call fast path **Goal:** make common function calls cheaper without changing language semantics. -The interpreter already snapshots `builtin_bindings`, but ordinary identifier call evaluation still goes through generic expression evaluation and `Value::NativeFunction` call handling. We should add a constrained fast path for simple identifier calls where the callee is a stable native/global function. +The interpreter already snapshots `builtin_bindings`, but ordinary identifier call evaluation still goes through generic expression evaluation and clones argument values before invoking `Value::NativeFunction`. A broad direct-call path for every stable native/global call is not automatically a win: it can spend more time proving the callee is unshadowed than it saves. The useful first slice is narrower and measurable: `len(identifier)` should not clone large arrays/maps just to compute their length. Scope: -- [ ] Profile common native calls in template/page workloads (`len`, string helpers, collections helpers, response builders, template filters). -- [ ] Add a direct-call path for `Expression::Call { function: Identifier(name), ... }` after server-action/template/fs special cases are handled. -- [ ] Avoid re-looking-up stable native functions through recursive environment chains when the name is known to be an unshadowed builtin/prelude binding. -- [ ] Preserve user-defined shadowing behavior. If an app defines `len`, the app binding must win. -- [ ] Add tests for shadowing, imported functions, prelude functions, and ordinary user functions. +- [x] Profile common native calls in template/page workloads (`len`, string helpers, collections helpers, response builders, template filters). +- [x] Add a targeted fast path for `len(identifier)` after preserving normal callee shadowing semantics. +- [x] Avoid cloning large identifier-bound arrays/maps when only their length is needed. +- [x] Preserve user-defined shadowing behavior. If an app defines `len`, the app binding must win. +- [x] Add tests for builtin behavior, shadowing, type errors, and parent-scope lookup behavior. Likely files: - `src/interpreter.rs` -- possibly a small call-dispatch helper to reduce the existing special-case ladder - focused interpreter tests +- `examples/perf/*` and benchmark harness docs for the native-call fixture Acceptance criteria: -- [ ] No behavior change for shadowing/imports. -- [ ] Simple native-call benchmark improves. -- [ ] Call dispatch code reads cleaner after the change, not more haunted. +- [x] No behavior change for shadowing/imports. +- [x] Simple native-call benchmark improves. +- [x] Call dispatch code stays localized rather than more haunted. + +Candidate benchmark note: local 3s/3-run `wrk` pass at 16 connections/2 threads, comparing `origin/main` plus the new `/native/calls` fixture to this PR's dev-release binary, showed `/native/calls` RPS `349.95 -> 1574.70` (+350.0%). Other routes stayed within normal local noise. ### PR 6: Environment lookup measurement + low-risk lookup cache @@ -505,7 +507,7 @@ Recommended local toolchain: - [x] PR 2 adds a repeatable benchmark harness and baseline results. - [x] PR 3 makes ordinary `template(path, data)` use a safe automatic AST/cache path. - [x] PR 4 reduces template render/loop scope overhead without semantic drift. -- [ ] PR 5 adds a safe direct native/global call fast path, or documents why profiling does not justify it. +- [x] PR 5 adds a safe targeted native/global call fast path for `len(identifier)`. - [ ] PR 6 adds lookup instrumentation/cache only if measurements justify it. - [ ] PR 7 cleans request/response allocation only if profiles show meaningful headroom. - [ ] DD-061 is updated after each merged PR with measured deltas and completed checkboxes. @@ -515,6 +517,4 @@ Recommended local toolchain: ## Current Recommendation -With the automatic template AST cache and loop-scope cleanup complete, use benchmark data to decide whether **PR 5: direct native/global call fast path** is justified next. Start by profiling common native calls in template/page workloads and only add the fast path if shadowing/import semantics can stay obvious and well-tested. - -If PR 5 does not produce a clean measurable win, skip to lookup instrumentation rather than forcing a dispatch optimization. Computers are annoyingly literal; we should let them tell us where they hurt. +With the automatic template AST cache, loop-scope cleanup, and targeted `len(identifier)` fast path complete, use benchmark/profile data to choose between **PR 6: environment lookup measurement/cache** and **PR 7: request/response allocation cleanup**. Do not force a broad generic native-call shortcut unless a profile shows it beats the extra shadowing checks. diff --git a/examples/perf/README.md b/examples/perf/README.md index a787227..2a53915 100644 --- a/examples/perf/README.md +++ b/examples/perf/README.md @@ -47,6 +47,7 @@ The DB fixtures use simple `SELECT` queries so they do not require schema setup. - `/json` — small JSON response - `/param/{id}` — route params and map reads - `/compute` — interpreter loop inside an HTTP request +- `/native/calls` — repeated stable native/global calls (`len`, `str`) inside an HTTP request - `/template/layout` — external template render with layout, partial, and loop - `/template/rows` — template-heavy 100-row render - `/db/single` — optional single PostgreSQL query diff --git a/examples/perf/server.tnt b/examples/perf/server.tnt index 901ac87..0ec8c19 100644 --- a/examples/perf/server.tnt +++ b/examples/perf/server.tnt @@ -54,6 +54,16 @@ fn compute_loop(req: Map) -> Map { return json(map { "total": total }) } +fn native_call_loop(req: Map) -> Map { + let mut total = 0 + let label = "ntnt-performance" + let rows = ROWS_10 + for i in 0..5000 { + total = total + len(label) + len(rows) + len(str(i)) + } + return json(map { "total": total }) +} + fn template_layout(req: Map) -> Map { let rendered = template("views/page.html", map { "title": "Template layout benchmark", @@ -121,6 +131,7 @@ get("/", plaintext) get("/json", small_json) get("/param/{id}", route_param) get("/compute", compute_loop) +get("/native/calls", native_call_loop) get("/template/layout", template_layout) get("/template/rows", template_rows) get("/db/single", db_single) diff --git a/scripts/bench/run-benchmarks.py b/scripts/bench/run-benchmarks.py index 457c320..a0ad884 100755 --- a/scripts/bench/run-benchmarks.py +++ b/scripts/bench/run-benchmarks.py @@ -36,6 +36,7 @@ {"name": "small JSON route", "path": "/json"}, {"name": "route param + map read", "path": "/param/12345"}, {"name": "compute loop route", "path": "/compute"}, + {"name": "native call loop route", "path": "/native/calls"}, {"name": "template layout + partial", "path": "/template/layout"}, {"name": "template 100-row loop", "path": "/template/rows"}, ] diff --git a/src/interpreter.rs b/src/interpreter.rs index 643815a..44d3e8e 100644 --- a/src/interpreter.rs +++ b/src/interpreter.rs @@ -399,11 +399,48 @@ impl Environment { pub fn get(&self, name: &str) -> Option { if let Some(value) = self.values.get(name) { - Some(value.clone()) - } else if let Some(ref parent) = self.parent { - parent.borrow().get(name) - } else { - None + return Some(value.clone()); + } + + let mut next = self.parent.clone(); + while let Some(env_rc) = next { + let env = env_rc.borrow(); + if let Some(value) = env.values.get(name) { + return Some(value.clone()); + } + next = env.parent.clone(); + } + + None + } + + fn len_of_binding(&self, name: &str) -> Option> { + if let Some(value) = self.values.get(name) { + return Some(Self::len_of_value(value)); + } + + let mut next = self.parent.clone(); + while let Some(env_rc) = next { + let env = env_rc.borrow(); + if let Some(value) = env.values.get(name) { + return Some(Self::len_of_value(value)); + } + next = env.parent.clone(); + } + + None + } + + fn len_of_value(value: &Value) -> Result { + match value { + Value::String(s) => Ok(Value::Int(s.len() as i64)), + Value::Array(a) => Ok(Value::Int(a.len() as i64)), + Value::Map(m) => Ok(Value::Int(m.len() as i64)), + other => Err(IntentError::type_error_with_context( + format!("len() requires a collection, got {}", other.type_name()), + TypeContext::new("String, Array, or Map", other.type_name()) + .with_hint("Use type(x) to check the type before calling len()"), + )), } } @@ -5930,6 +5967,24 @@ impl Interpreter { } => { // Special handling for old() in postconditions if let Expression::Identifier(name) = function.as_ref() { + // Fast path for len(identifier): avoid cloning large arrays/maps just to + // compute their length. This preserves normal function shadowing by only + // firing when `len` currently resolves to the builtin native function. + if name == "len" && arguments.len() == 1 { + if let Expression::Identifier(arg_name) = &arguments[0] { + let len_is_builtin = matches!( + self.environment.borrow().get("len"), + Some(Value::NativeFunction { name, .. }) if name == "len" + ); + if len_is_builtin { + let len_result = self.environment.borrow().len_of_binding(arg_name); + if let Some(result) = len_result { + return result; + } + } + } + } + if name == "old" && arguments.len() == 1 { // Look up the pre-execution value let key = format!("{:?}", &arguments[0]); @@ -10747,6 +10802,101 @@ mod tests { )) } + #[test] + fn environment_get_finds_parent_bindings_and_preserves_child_shadowing() { + let root = Rc::new(RefCell::new(Environment::new())); + root.borrow_mut() + .define("name".to_string(), Value::String("root".to_string())); + root.borrow_mut() + .define("only_root".to_string(), Value::Int(7)); + + let child = Environment::with_parent(Rc::clone(&root)); + assert!(matches!(child.get("only_root"), Some(Value::Int(7)))); + assert!(matches!(child.get("name"), Some(Value::String(ref s)) if s == "root")); + + let mut shadowing_child = Environment::with_parent(root); + shadowing_child.define("name".to_string(), Value::String("child".to_string())); + assert!(matches!(shadowing_child.get("name"), Some(Value::String(ref s)) if s == "child")); + assert!(shadowing_child.get("missing").is_none()); + } + + #[test] + fn environment_get_handles_deep_scope_chains_iteratively() { + let root = Rc::new(RefCell::new(Environment::new())); + root.borrow_mut() + .define("target".to_string(), Value::Int(42)); + + let mut current = root; + for _ in 0..64 { + current = Rc::new(RefCell::new(Environment::with_parent(current))); + } + + assert!(matches!( + current.borrow().get("target"), + Some(Value::Int(42)) + )); + } + + #[test] + fn environment_len_of_binding_handles_parent_scopes() { + let root = Rc::new(RefCell::new(Environment::new())); + root.borrow_mut().define( + "rows".to_string(), + Value::Array(vec![Value::Int(1), Value::Int(2), Value::Int(3)]), + ); + + let child = Environment::with_parent(Rc::new(RefCell::new(Environment::with_parent(root)))); + assert!(matches!( + child.len_of_binding("rows"), + Some(Ok(Value::Int(3))) + )); + } + + #[test] + fn len_identifier_fast_path_preserves_builtin_behavior() { + let result = eval( + r#" + let rows = [1, 2, 3, 4] + len(rows) + "#, + ) + .unwrap(); + + assert!(matches!(result, Value::Int(4))); + } + + #[test] + fn len_identifier_fast_path_preserves_shadowed_len_function() { + let result = eval( + r#" + fn len(value) { + return 99 + } + + let rows = [1, 2, 3, 4] + len(rows) + "#, + ) + .unwrap(); + + assert!(matches!(result, Value::Int(99))); + } + + #[test] + fn len_identifier_fast_path_preserves_type_errors() { + let result = eval( + r#" + let n = 123 + len(n) + "#, + ) + .unwrap_err(); + + assert!( + matches!(result, IntentError::TypeError { message, .. } if message.contains("len() requires a collection")) + ); + } + #[test] fn template_cache_reuses_parsed_templates_and_partials() { let dir = unique_template_test_dir("reuse");