Move constant values of map literals into the template

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

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<Identifier, Dynamic>),
+    ) -> 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]

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)

src/eval/mod.rs

@@ -24,6 +24,10 @@
 #[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"))]
@@ -39,7 +43,7 @@
 pub use eval_context::EvalContext;
 #[cfg(not(feature = "no_ast"))]
 pub use eval_context::EvalContextFrameGuard;
 pub use switch::RangeCase;
 
 #[cfg(not(feature = "no_function"))]
 pub use global_state::GlobalRef;

src/grain/compile/mod.rs

@@ -1711,8 +1711,8 @@
 
             // 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.
@@ -2817,7 +2817,7 @@
 #[cfg(not(feature = "no_function"))]
 mod tests {
     use super::*;
     use crate::grain::bytecode::StepFlags;
 
     /// Lowering order fixes every address inside a function, so it has to come
     /// from the source rather than from a hash map.

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,
             );
 

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;

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)

tests/data_size.rs

@@ -1,5 +1,5 @@
 #![cfg(not(feature = "unchecked"))]
 use rhai::{Engine, EvalAltResult, ParseErrorType, INT};
 
 #[cfg(not(feature = "no_index"))]
 use rhai::Array;
@@ -33,6 +33,20 @@
         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 @@
         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(

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::<Dynamic>(&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::<Dynamic>(&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]

tests/maps.rs

@@ -1,5 +1,5 @@
 #![cfg(not(feature = "no_object"))]
 use rhai::{Engine, EvalAltResult, Map, ParseErrorType, Scope, INT};
 
 #[test]
 fn test_map_indexing() {
@@ -285,3 +285,16 @@
         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::<INT>("#{a: 1, b: 2 + 3}.a").unwrap(), 1);
+    assert_eq!(engine.eval::<INT>("#{a: 1, b: 2 + 3}.b").unwrap(), 5);
+    #[cfg(not(feature = "no_index"))]
+    assert_eq!(engine.eval::<INT>(r#"#{a: 1, b: 2 + 3}["a"]"#).unwrap(), 1);
+    assert_eq!(engine.eval::<INT>("let x = 4; let m = #{a: 1, b: x, c: 3}; m.a + m.b + m.c").unwrap(), 8);
+}

tests/modules.rs

@@ -499,10 +499,10 @@
         let engine = context.engine();
 
         // Store context for later use - requires the 'internals' feature
         let context_data = context.store_data();
 
         // Recreate the 'NativeCallContext'
         let new_context = context_data.create_context(engine);
 
         fp.call_within_context(&new_context, (41 as INT,))
     });
@@ -587,6 +587,26 @@
     assert_eq!(f.call::<String>(&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::<rhai::Map>(&ast).unwrap();
+    let f = map["f"].clone_cast::<rhai::FnPtr>();
+
+    assert_eq!(f.call::<String>(&engine, &ast, ()).unwrap(), "from m");
+}
+
 #[cfg(not(feature = "no_function"))]
 #[test]
 fn text_module_constant_store() {