From 1c485b37640abb7b6f242af534ceb551e6f5cd29 Mon Sep 17 00:00:00 2001 From: Keavon Chambers Date: Tue, 22 Sep 2026 01:21:40 -0700 Subject: [PATCH] Parse each expression node's source once, reusing the tree across list items and frames --- .../messages/portfolio/document_migration.rs | 2 +- node-graph/nodes/gcore/src/memo.rs | 4 +- node-graph/nodes/math/src/lib.rs | 83 +++++++++++++------ 3 files changed, 62 insertions(+), 27 deletions(-) diff --git a/editor/src/messages/portfolio/document_migration.rs b/editor/src/messages/portfolio/document_migration.rs index 435b8d7e28b..99dd84fd490 100644 --- a/editor/src/messages/portfolio/document_migration.rs +++ b/editor/src/messages/portfolio/document_migration.rs @@ -1260,7 +1260,7 @@ pub fn document_migration_upgrades(document: &mut DocumentMessageHandler, reset_ } } - // The old "Math" node evaluated an expression over "A" and "B". A static expression not reading a wired `B` becomes "Math f(x)" + // The old "Math" node evaluated an expression over "A" and "B". A lexable static expression not reading a wired `B` becomes "Math f(x)" // with `A` as `x` and a constant `B` inlined. Anything else becomes "Extend" feeding "Math f(…)", which reads the pair as `a` and `b`. let math_nodes: Vec<(NodeId, Vec, Vec)> = document .network_interface diff --git a/node-graph/nodes/gcore/src/memo.rs b/node-graph/nodes/gcore/src/memo.rs index cc5befe0e23..e51c3522e16 100644 --- a/node-graph/nodes/gcore/src/memo.rs +++ b/node-graph/nodes/gcore/src/memo.rs @@ -10,7 +10,7 @@ use std::sync::Mutex; /// /// Stores the last evaluated data that flowed through this node and immediately returns that data on subsequent renders if the context has not changed. #[node_macro::node(category("General"), path(graphene_core::memo), skip_impl)] -async fn memoize(input: I, #[data] cache: Arc>>, content: impl Node) -> T { +async fn memoize(input: I, content: impl Node, #[data] cache: Arc>>) -> T { // Caches the output of a given node called with a specific input. // // A cache miss occurs when the Option is None. In this case, the node evaluates the inner node and memoizes (stores) the result. @@ -38,10 +38,10 @@ type MonitorValue = Arc>>>>; #[node_macro::node(category(""), path(graphene_core::memo), serialize(serialize_monitor), properties("monitor_properties"), skip_impl)] async fn monitor( input: I, + content: impl Node, #[allow(clippy::type_complexity)] #[data] io: MonitorValue, - content: impl Node, ) -> T { let output = content.eval(input.clone()).await; *io.lock().unwrap() = Some(Arc::new(IORecord { input, output: output.clone() })); diff --git a/node-graph/nodes/math/src/lib.rs b/node-graph/nodes/math/src/lib.rs index 3c94dde84fb..92fd6092c45 100644 --- a/node-graph/nodes/math/src/lib.rs +++ b/node-graph/nodes/math/src/lib.rs @@ -15,19 +15,43 @@ use math_parser::reducer::classify_reducer; use math_parser::value::Value; use rand::{Rng, SeedableRng}; use std::ops::{Add, Mul, Rem, Sub}; +use std::sync::{Arc, Mutex, PoisonError}; use vector_types::Gradient; -/// Parses and evaluates a math expression with the given variable bindings, logging and returning `None` on failure. -fn evaluate_expression(expression: &str, provider: impl ValueProvider) -> Option { - let node = match ast::Node::try_parse_from_str(expression) { - Ok(node) => node, - Err(error) => { - warn!("Invalid expression: `{expression}`\n{error}"); - return None; +/// A parsed source and its tree, which an invalid source lacks. +type ParsedSource = Option<(String, Option>)>; + +/// The last expression a node parsed, reused while its source stays the same, so a list of thousands of items or a run of +/// frames parses once. An invalid source is remembered too, so it is logged once rather than per item. +#[derive(Debug, Clone, Default)] +pub struct ParseCache(Arc>); + +impl ParseCache { + /// The parse tree of `source`, or `None` for an invalid expression. + fn parse(&self, source: &str) -> Option> { + // A lock poisoned by a panic elsewhere still guards a usable cache + let mut cached = self.0.lock().unwrap_or_else(PoisonError::into_inner); + if let Some((cached_source, tree)) = cached.as_ref() + && cached_source == source + { + return tree.clone(); } - }; - match node.eval(&EvalContext::new(provider, NothingMap)) { + let tree = match ast::Node::try_parse_from_str(source) { + Ok(tree) => Some(Arc::new(tree)), + Err(error) => { + warn!("Invalid expression: `{source}`\n{error}"); + None + } + }; + *cached = Some((source.to_string(), tree.clone())); + tree + } +} + +/// Evaluates a parsed expression with the given variable bindings, logging and returning `None` on failure. +fn evaluate_expression(expression: &ast::Node, provider: impl ValueProvider) -> Option { + match expression.eval(&EvalContext::new(provider, NothingMap)) { Ok(value) => Some(value), Err(error) => { warn!("Expression evaluation error: {error:?}"); @@ -110,11 +134,12 @@ fn math_fx( #[name("f(x) =")] #[default("x")] fx: Item, + #[data] parsed: ParseCache, ) -> Item { let (value, attributes) = value.into_parts(); let x = value.into_f64(); - let result = output(evaluate_expression(fx.element(), SingleVariableMathContext { x })); + let result = output(parsed.parse(fx.element()).and_then(|expression| evaluate_expression(&expression, SingleVariableMathContext { x }))); Item::from_parts(result, attributes) } @@ -155,6 +180,7 @@ fn math_f( /// The expression evaluated over the items, such as `a * b + c`, or a lone operator or function applied across all of them. #[name("f(…) =")] f: Item, + #[data] parsed: ParseCache, ) -> Item { let expression = f.element(); let items: Vec = values.iter_element_values().map(|&value| value.into_f64()).collect(); @@ -169,7 +195,7 @@ fn math_f( return Item::new_from_element(output(Some(Value::from_f64(result)))); } - let result = output(evaluate_expression(expression, bindings)); + let result = output(parsed.parse(expression).and_then(|expression| evaluate_expression(&expression, bindings))); Item::new_from_element(result) } @@ -1919,41 +1945,50 @@ mod test { #[test] fn test_basic_expression() { - let result = math_fx((), Item::new_from_element(0.), Item::new_from_element("2 + 2".to_string())); + let result = math_fx((), &ParseCache::default(), Item::new_from_element(0.), Item::new_from_element("2 + 2".to_string())); assert_eq!(result.into_element(), 4.); } #[test] fn test_complex_expression() { - let result = math_fx((), Item::new_from_element(0.), Item::new_from_element("(5 * 3) + (10 / 2)".to_string())); + let result = math_fx((), &ParseCache::default(), Item::new_from_element(0.), Item::new_from_element("(5 * 3) + (10 / 2)".to_string())); assert_eq!(result.into_element(), 20.); } #[test] fn test_variable_binding() { - let result = math_fx((), Item::new_from_element(7.), Item::new_from_element("x * 2".to_string())); + let result = math_fx((), &ParseCache::default(), Item::new_from_element(7.), Item::new_from_element("x * 2".to_string())); assert_eq!(result.into_element(), 14.); } #[test] fn test_invalid_expression() { - let result = math_fx((), Item::new_from_element(0.), Item::new_from_element("invalid".to_string())); + let result = math_fx((), &ParseCache::default(), Item::new_from_element(0.), Item::new_from_element("invalid".to_string())); assert_eq!(result.into_element(), 0.); } + #[test] + fn expressions_parse_once_per_source() { + let cache = ParseCache::default(); + let first = cache.parse("x * 2").unwrap(); + assert!(Arc::ptr_eq(&first, &cache.parse("x * 2").unwrap())); + assert!(!Arc::ptr_eq(&first, &cache.parse("x * 3").unwrap())); + assert!(cache.parse("invalid(").is_none()); + } + #[test] fn test_boolean_items() { // Booleans read as exactly 0 and 1, and logical results convert back - assert!(!math_fx((), Item::new_from_element(true), Item::new_from_element("!x".to_string())).into_element()); - assert!(math_fx((), Item::new_from_element(false), Item::new_from_element("x == 0".to_string())).into_element()); + assert!(!math_fx((), &ParseCache::default(), Item::new_from_element(true), Item::new_from_element("!x".to_string())).into_element()); + assert!(math_fx((), &ParseCache::default(), Item::new_from_element(false), Item::new_from_element("x == 0".to_string())).into_element()); // A result that is not exactly 0 or 1 cannot be a truth value, so it reads as false - assert!(!math_fx((), Item::new_from_element(true), Item::new_from_element("x + 1".to_string())).into_element()); + assert!(!math_fx((), &ParseCache::default(), Item::new_from_element(true), Item::new_from_element("x + 1".to_string())).into_element()); let bools = || [true, true, false].into_iter().map(Item::new_from_element).collect::>(); - assert!(!math_f((), bools(), Item::new_from_element("&&".to_string())).into_element()); - assert!(math_f((), bools(), Item::new_from_element("||".to_string())).into_element()); - assert!(!math_f((), bools(), Item::new_from_element("xor".to_string())).into_element()); + assert!(!math_f((), &ParseCache::default(), bools(), Item::new_from_element("&&".to_string())).into_element()); + assert!(math_f((), &ParseCache::default(), bools(), Item::new_from_element("||".to_string())).into_element()); + assert!(!math_f((), &ParseCache::default(), bools(), Item::new_from_element("xor".to_string())).into_element()); } #[test] @@ -1974,9 +2009,9 @@ mod test { let values = || [4., 1., 7.].into_iter().map(Item::new_from_element).collect::>(); // A full expression reads the items positionally as `a`, `b`, `c`, while a lone token applies across all of them - assert_eq!(math_f((), values(), Item::new_from_element("a - b + c".to_string())).into_element(), 10.); - assert_eq!(math_f((), values(), Item::new_from_element("min".to_string())).into_element(), 1.); - assert_eq!(math_f((), values(), Item::new_from_element("+".to_string())).into_element(), 12.); + assert_eq!(math_f((), &ParseCache::default(), values(), Item::new_from_element("a - b + c".to_string())).into_element(), 10.); + assert_eq!(math_f((), &ParseCache::default(), values(), Item::new_from_element("min".to_string())).into_element(), 1.); + assert_eq!(math_f((), &ParseCache::default(), values(), Item::new_from_element("+".to_string())).into_element(), 12.); } #[test]