From 489b8a923022a01a94a7e09ef6eca297d40cd6e5 Mon Sep 17 00:00:00 2001 From: Luis Pedro Coelho Date: Wed, 28 Oct 2026 16:02:51 +1000 Subject: [PATCH] Move constant values of map literals into the template A map literal with any computed value is not folded into a constant, and every entry, constant or not, was evaluated each time the literal was. After optimizing a map literal's entries, the optimizer now moves the constant values into the literal's template, so only the computed ones are evaluated. A function pointer stays an entry, as it takes the environment it is evaluated in, and so does a shared value, which is flattened as it is evaluated. The promotion of `#{..}.a` and `#{..}["a"]` looks in the template for a moved value. The walker and the VM start a map literal's running data size from its template, so the constant values still count towards the limits. The parser checks for a duplicated property in the template rather than scanning the entries. At OptimizationLevel::None nothing moves, so constant entries still count as operations there. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + src/ast/expr.rs | 56 +++++++++++++++++++++++++++++++++++++--- src/eval/expr.rs | 10 +++++-- src/eval/mod.rs | 4 +++ src/grain/compile/mod.rs | 4 +-- src/grain/vm/mod.rs | 18 +++++++++++-- src/optimizer.rs | 18 ++++++++++--- src/parser.rs | 2 +- tests/data_size.rs | 27 +++++++++++++++++++ tests/grain/limits.rs | 31 ++++++++++++++++++++++ tests/maps.rs | 13 ++++++++++ tests/modules.rs | 20 ++++++++++++++ 12 files changed, 191 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 08f2ebd50..d9fb380be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ Enhancements * A `Module` created by `Module::eval_grain_as_new` now looks up the functions of the Rhai Grain program only when called. The new `Vm::eval_with_lazy_callbacks` does the same for functions called back from native Rust functions, while `Vm::eval_with_callbacks` still registers every function up-front (thanks [`@ImTheSquid`](https://github.com/ImTheSquid) [`#1192`](https://github.com/rhaiscript/rhai/pull/1192)). * Function call dispatches now run faster due to optimization inlining a fast path (thanks [`@xorza`](https://github.com/xorza) [`#1199`](https://github.com/rhaiscript/rhai/pull/1199)). * Operations counting is optimized to run fast by avoiding atomics when not building under `sync` (thanks [`@xorza`](https://github.com/xorza) [`#1198`](https://github.com/rhaiscript/rhai/pull/1198)). +* Object map literals with mostly constant values evaluate faster: the optimizer moves the constant values into the literal's template, so only the computed values are evaluated each time ([`#1196`](https://github.com/rhaiscript/rhai/issues/1196)). As for a map literal that is all constant, its constant values no longer count as operations. Under `internals`, the entries of an optimized `Expr::Map` hold only the computed values. Version 1.27.0 diff --git a/src/ast/expr.rs b/src/ast/expr.rs index 0ed0c2d25..0969832ae 100644 --- a/src/ast/expr.rs +++ b/src/ast/expr.rs @@ -261,9 +261,17 @@ impl fmt::Debug for Expr { } Self::Map(x, ..) => { f.write_str("Map")?; - f.debug_map() - .entries(x.0.iter().map(|(k, v)| (k, v))) - .finish() + // The entries are the computed values; the template holds the constant ones. + let computed: BTreeMap<_, _> = + x.0.iter().map(|(k, v)| (k.as_str(), (k, v))).collect(); + let mut map = f.debug_map(); + for (key, value) in &x.1 { + match computed.get(key.as_str()) { + Some((k, v)) => map.entry(k, v), + None => map.entry(key, value), + }; + } + map.finish() } Self::ThisPtr(..) => f.debug_struct("ThisPtr").finish(), Self::Variable(x, i, ..) => { @@ -461,6 +469,48 @@ impl Expr { _ => return None, }) } + /// Move the constant values of a map literal's entries into its template, which holds `()` + /// for each until then, so they are not evaluated again each time the literal is. + /// + /// Returns `true` if any entry moved. + /// + /// A function pointer stays an entry: evaluating it gives it the environment it is + /// evaluated in. So does a shared value, which is flattened as it is evaluated. + #[cfg(not(feature = "no_object"))] + #[cfg(not(feature = "no_optimize"))] + pub(crate) fn lift_map_constants( + map: &mut (StaticVec<(Ident, Self)>, BTreeMap), + ) -> bool { + fn is_template_value(value: &Dynamic) -> bool { + match value.0 { + Union::FnPtr(..) => false, + #[cfg(not(feature = "no_closure"))] + Union::Shared(..) => false, + #[cfg(not(feature = "no_index"))] + Union::Array(ref a, ..) => a.iter().all(is_template_value), + Union::Map(ref m, ..) => m.values().all(is_template_value), + _ => true, + } + } + + let (entries, template) = map; + let len = entries.len(); + + entries.retain(|(key, expr)| { + if !expr.is_constant() { + return true; + } + match (expr.get_literal_value(None), template.get_mut(key.as_str())) { + (Some(value), Some(slot)) if is_template_value(&value) => { + *slot = value; + false + } + _ => true, + } + }); + + entries.len() < len + } /// Create an [`Expr`] from a [`Dynamic`] value. #[inline] #[must_use] diff --git a/src/eval/expr.rs b/src/eval/expr.rs index d200a4eca..749e351d2 100644 --- a/src/eval/expr.rs +++ b/src/eval/expr.rs @@ -309,8 +309,14 @@ impl Engine { Expr::Map(x, ..) => { let mut map = x.1.clone(); + // The template holds the constant values, and a `()` for each computed one, so + // it counts every entry once. #[cfg(not(feature = "unchecked"))] - let mut total_data_sizes = (0, 0, 0); + let mut total_data_sizes = if self.has_data_size_limit() && !x.0.is_empty() { + crate::eval::calc_map_sizes(&x.1) + } else { + (0, 0, 0) + }; for (key, value_expr) in &x.0 { let value = self @@ -322,7 +328,7 @@ impl Engine { let delta = crate::eval::calc_data_sizes(&value, true); total_data_sizes = ( total_data_sizes.0 + delta.0, - total_data_sizes.1 + delta.1 + 1, + total_data_sizes.1 + delta.1, total_data_sizes.2 + delta.2, ); self.throw_on_size(total_data_sizes) diff --git a/src/eval/mod.rs b/src/eval/mod.rs index c6298eefa..6ba44d681 100644 --- a/src/eval/mod.rs +++ b/src/eval/mod.rs @@ -24,6 +24,10 @@ pub use data_check::calc_array_sizes; #[cfg(not(feature = "unchecked"))] #[cfg(any(not(feature = "no_index"), not(feature = "no_object")))] pub use data_check::calc_data_sizes; +#[cfg(not(feature = "unchecked"))] +#[cfg(not(feature = "no_object"))] +#[cfg(not(feature = "no_ast"))] +pub use data_check::calc_map_sizes; #[cfg(feature = "debugging")] #[cfg(not(feature = "no_function"))] #[cfg(not(feature = "no_ast"))] diff --git a/src/grain/compile/mod.rs b/src/grain/compile/mod.rs index e26760950..e794ad073 100644 --- a/src/grain/compile/mod.rs +++ b/src/grain/compile/mod.rs @@ -1711,8 +1711,8 @@ impl Lowering { // The other half of the same shape. Rhai keeps a map literal as a // template holding every key — the constant values already in - // place, the computed ones as placeholders — plus the list of - // entries still to evaluate. + // place once optimized, the others as `()` placeholders — plus the + // list of entries still to evaluate. // // An all-constant map is folded into a `DynamicConstant` and never // arrives here; one with a single computed value does. diff --git a/src/grain/vm/mod.rs b/src/grain/vm/mod.rs index db812e8b4..3d95d4c32 100644 --- a/src/grain/vm/mod.rs +++ b/src/grain/vm/mod.rs @@ -3516,7 +3516,21 @@ impl<'e> Vm<'e> { .last() .ok_or_else(|| malformed("size check with no element".to_string()))?; - let delta = calc_data_sizes(value, true); + let mut delta = calc_data_sizes(value, true); + + // A map literal's template, under the first entry's key and value, + // holds the constant values and a `()` for each computed one, so it + // counts every entry once, as Rhai does. + if map && index == 0 { + let template = self + .stack + .len() + .checked_sub(3) + .and_then(|at| self.stack.get(at)) + .ok_or_else(|| malformed("map size check without a template".to_string()))?; + let sizes = calc_data_sizes(template, true); + delta = (delta.0 + sizes.0, delta.1 + sizes.1, delta.2 + sizes.2); + } let total = self .sizes @@ -3525,7 +3539,7 @@ impl<'e> Vm<'e> { *total = ( total.0 + delta.0 + usize::from(!map), - total.1 + delta.1 + usize::from(map), + total.1 + delta.1, total.2 + delta.2, ); diff --git a/src/optimizer.rs b/src/optimizer.rs index 4d4954ea1..d942ae3fe 100644 --- a/src/optimizer.rs +++ b/src/optimizer.rs @@ -976,7 +976,10 @@ fn optimize_expr(expr: &mut Expr, state: &mut OptimizerState, _chaining: bool) { // All other items can be thrown away. state.set_dirty(); *expr = mem::take(&mut m.0).into_iter().find(|(x, ..)| x.name == p.2) - .map_or_else(|| Expr::Unit(*pos), |(.., mut expr)| { expr.set_position(*pos); expr }); + .map(|(.., mut expr)| { expr.set_position(*pos); expr }) + // A constant value is in the template. + .or_else(|| m.1.get(p.2.as_str()).map(|v| Expr::from_dynamic(v.clone(), *pos))) + .unwrap_or(Expr::Unit(*pos)); } // var.property where var is a constant map (Expr::Variable(v, _, pos) , Expr::Property(p, ..)) if state.propagate_constants && state.find_literal_constant(&v.1).map_or(false,Dynamic::is_map) => { @@ -1043,7 +1046,10 @@ fn optimize_expr(expr: &mut Expr, state: &mut OptimizerState, _chaining: bool) { // All other items can be thrown away. state.set_dirty(); *expr = mem::take(&mut m.0).into_iter().find(|(x, ..)| x.name == s) - .map_or_else(|| Expr::Unit(*pos), |(.., mut expr)| { expr.set_position(*pos); expr }); + .map(|(.., mut expr)| { expr.set_position(*pos); expr }) + // A constant value is in the template. + .or_else(|| m.1.get(s.as_str()).map(|v| Expr::from_dynamic(v.clone(), *pos))) + .unwrap_or(Expr::Unit(*pos)); } #[cfg(not(feature = "no_object"))] (Expr::DynamicConstant(cst, pos ), Expr::StringConstant(s, ..)) if cst.is_map() => { @@ -1143,7 +1149,13 @@ fn optimize_expr(expr: &mut Expr, state: &mut OptimizerState, _chaining: bool) { } // #{ key:value, .. } #[cfg(not(feature = "no_object"))] - Expr::Map(x, ..) => x.0.iter_mut().for_each(|(.., expr)| optimize_expr(expr, state, false)), + Expr::Map(x, ..) => { + x.0.iter_mut().for_each(|(.., expr)| optimize_expr(expr, state, false)); + // Values that have become constant join the template. + if Expr::lift_map_constants(x) { + state.set_dirty(); + } + } // lhs && rhs Expr::And(x, ..) => { let mut is_false = None; diff --git a/src/parser.rs b/src/parser.rs index 7d94d4022..64a9f5652 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -1036,7 +1036,7 @@ impl Engine { return Err(PERR::PropertyExpected.into_err(pos)) } (Token::Identifier(s) | Token::StringConstant(s), pos) => { - if map.iter().any(|(p, ..)| p.as_str() == s.as_str()) { + if template.contains_key(s.as_str()) { return Err(PERR::DuplicatedProperty(s.to_string()).into_err(pos)); } (*s, pos) diff --git a/tests/data_size.rs b/tests/data_size.rs index 4285def0c..2ada9293a 100644 --- a/tests/data_size.rs +++ b/tests/data_size.rs @@ -33,6 +33,20 @@ fn test_max_string_size() { EvalAltResult::ErrorDataTooLarge(..) )); + // The constant values of a map literal count towards its total, too. + #[cfg(not(feature = "no_object"))] + assert!(matches!( + *engine + .run( + r#" + let x = "abc"; + #{a: "abcdef", b: "ghijk", c: x} + "# + ) + .unwrap_err(), + EvalAltResult::ErrorDataTooLarge(..) + )); + #[cfg(not(feature = "no_object"))] assert!(matches!( *engine @@ -282,6 +296,19 @@ fn test_max_map_size() { EvalAltResult::ErrorDataTooLarge(..) )); + // The constant values of a map literal count towards its total, too. + assert!(matches!( + *engine + .run( + " + let x = 1; + #{a: #{b:1,c:2,d:3,e:4,f:5}, g: #{h:1,i:2,j:3,k:4}, l: x} + " + ) + .unwrap_err(), + EvalAltResult::ErrorDataTooLarge(..) + )); + assert!(matches!( *engine .run( diff --git a/tests/grain/limits.rs b/tests/grain/limits.rs index 51d0939c6..23392c83c 100644 --- a/tests/grain/limits.rs +++ b/tests/grain/limits.rs @@ -105,6 +105,37 @@ fn interpolation_respects_the_string_limit() { assert_eq!(format!("{vm:?}"), format!("{walker:?}"), "including the position of the segment that went over",); } +/// A map literal's constant values are in its template rather than evaluated, +/// but they still count towards the literal's total: the VM must refuse what +/// the walker refuses, against the same entry. +#[test] +#[cfg(not(feature = "no_object"))] +fn map_constants_count_towards_the_limits() { + let mut engine = Engine::new(); + engine.set_max_string_size(10); + engine.set_max_map_size(10); + + for source in [r#"let x = "abc"; #{a: "abcdef", b: "ghijk", c: x}"#, r#"let x = 1; #{a: #{b:1,c:2,d:3,e:4,f:5}, g: #{h:1,i:2,j:3,k:4}, l: x}"#] { + let ast = engine.compile(source).expect("must compile"); + let program = Compiler::new().compile(&ast); + assert_eq!(program.residual_count(), 0, "{source:?} must be lowered, not walked"); + + let walker = engine.eval_ast_with_scope::(&mut Scope::new(), &ast).expect_err("the walker must refuse it"); + let vm = Vm::new(&engine).eval_with_scope(&mut Scope::new(), &program).expect_err("and so must the VM"); + + assert!(matches!(*vm, EvalAltResult::ErrorDataTooLarge(..)), "{source:?}: got {vm:?}"); + assert_eq!(format!("{vm:?}"), format!("{walker:?}"), "{source:?}"); + } + + // Under the limits, they agree on the value. + let source = r#"let x = "abc"; #{a: "ab", b: #{c: 1}, d: x}"#; + let ast = engine.compile(source).expect("must compile"); + let program = Compiler::new().compile(&ast); + let walker = engine.eval_ast_with_scope::(&mut Scope::new(), &ast).unwrap(); + let vm = Vm::new(&engine).eval_with_scope(&mut Scope::new(), &program).unwrap(); + assert_eq!(format!("{vm:?}"), format!("{walker:?}")); +} + /// A loop that does terminate must not be killed by the tick itself, and must /// still produce the value Rhai produces. #[test] diff --git a/tests/maps.rs b/tests/maps.rs index c3b0dd9b1..482ba3715 100644 --- a/tests/maps.rs +++ b/tests/maps.rs @@ -285,3 +285,16 @@ fn test_map_missing_property_callback() { 143 ); } + +/// A map literal's constant values move into its template, which the optimizer +/// still reads when it promotes the property taken from the literal. +#[test] +fn test_map_constants_in_template() { + let engine = Engine::new(); + + assert_eq!(engine.eval::("#{a: 1, b: 2 + 3}.a").unwrap(), 1); + assert_eq!(engine.eval::("#{a: 1, b: 2 + 3}.b").unwrap(), 5); + #[cfg(not(feature = "no_index"))] + assert_eq!(engine.eval::(r#"#{a: 1, b: 2 + 3}["a"]"#).unwrap(), 1); + assert_eq!(engine.eval::("let x = 4; let m = #{a: 1, b: x, c: 3}; m.a + m.b + m.c").unwrap(), 8); +} diff --git a/tests/modules.rs b/tests/modules.rs index 189605bf1..9facfbc5d 100644 --- a/tests/modules.rs +++ b/tests/modules.rs @@ -587,6 +587,26 @@ fn test_module_closure_keeps_imports() { assert_eq!(f.call::(&engine, &ast, ()).unwrap(), "from m"); } +#[cfg(not(feature = "no_function"))] +#[cfg(not(feature = "no_object"))] +#[test] +fn test_module_closure_in_map_keeps_imports() { + let mut engine = Engine::new(); + + let ast = engine.compile(r#"fn who() { "from m" }"#).unwrap(); + let mut resolver = StaticModuleResolver::new(); + resolver.insert("m", Module::eval_ast_as_new(Scope::new(), &ast, &engine).unwrap()); + engine.set_module_resolver(resolver); + + // A closure is a constant value of the map literal, but it takes its environment as the + // literal is evaluated, so it is not moved into the literal's template. + let ast = engine.compile(r#"import "m" as m; let x = 1; #{ f: || m::who(), x: x }"#).unwrap(); + let map = engine.eval_ast::(&ast).unwrap(); + let f = map["f"].clone_cast::(); + + assert_eq!(f.call::(&engine, &ast, ()).unwrap(), "from m"); +} + #[cfg(not(feature = "no_function"))] #[test] fn text_module_constant_store() {