diff --git a/CHANGELOG.md b/CHANGELOG.md index ac63f7ed..bc700e7c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ Enhancements * Variable definitions are optimized so that shadowed variables are reused as much as possible to reduce memory consumption. * `FnAccess` and `FnNamespace` now implement `Ord` and `PartialOrd`. * The `event_handler_map` example is enhanced to prevent shadowing of the state object map. +* Separation of constants in function calls is removed as its performance benefit is dubious. Version 1.5.0 diff --git a/src/ast/expr.rs b/src/ast/expr.rs index af352bdf..a72b2540 100644 --- a/src/ast/expr.rs +++ b/src/ast/expr.rs @@ -176,17 +176,6 @@ pub struct FnCallExpr { pub hashes: FnCallHashes, /// List of function call argument expressions. pub args: StaticVec, - /// List of function call arguments that are constants. - /// - /// Any arguments in `args` that is [`Expr::Stack`] indexes into this - /// array to find the constant for use as its argument value. - /// - /// # Notes - /// - /// Constant arguments are very common in function calls, and keeping each constant in - /// an [`Expr::DynamicConstant`] involves an additional allocation. Keeping the constant - /// values in an inlined array avoids these extra allocations. - pub constants: StaticVec, /// Does this function call capture the parent scope? pub capture_parent_scope: bool, /// [Position] of the function name. @@ -206,9 +195,6 @@ impl fmt::Debug for FnCallExpr { ff.field("hash", &self.hashes) .field("name", &self.name) .field("args", &self.args); - if !self.constants.is_empty() { - ff.field("constants", &self.constants); - } ff.field("pos", &self.pos); ff.finish() } @@ -409,12 +395,6 @@ pub enum Expr { ), /// xxx `.` method `(` expr `,` ... `)` MethodCall(Box, Position), - /// Stack slot for function calls. See [`FnCallExpr`] for more details. - /// - /// This variant does not map to any language structure. It is used in function calls with - /// constant arguments where the `usize` number indexes into an array containing a list of - /// constant arguments for the function call. - Stack(usize, Position), /// { [statement][Stmt] ... } Stmt(Box), /// func `(` expr `,` ... `)` @@ -490,7 +470,6 @@ impl fmt::Debug for Expr { } Self::Property(x, ..) => write!(f, "Property({})", x.2), Self::MethodCall(x, ..) => f.debug_tuple("MethodCall").field(x).finish(), - Self::Stack(x, ..) => write!(f, "ConstantArg[{}]", x), Self::Stmt(x) => { let pos = x.span(); if !pos.is_none() { @@ -645,8 +624,7 @@ impl Expr { namespace: None, name: KEYWORD_FN_PTR.into(), hashes: calc_fn_hash(f.fn_name(), 1).into(), - args: once(Self::Stack(0, pos)).collect(), - constants: once(f.fn_name().into()).collect(), + args: once(Self::StringConstant(f.fn_name().into(), pos)).collect(), capture_parent_scope: false, pos, } @@ -704,7 +682,6 @@ impl Expr { | Self::Array(.., pos) | Self::Map(.., pos) | Self::Variable(.., pos, _) - | Self::Stack(.., pos) | Self::And(.., pos) | Self::Or(.., pos) | Self::Index(.., pos) @@ -759,7 +736,6 @@ impl Expr { | Self::Dot(.., pos) | Self::Index(.., pos) | Self::Variable(.., pos, _) - | Self::Stack(.., pos) | Self::FnCall(.., pos) | Self::MethodCall(.., pos) | Self::Custom(.., pos) @@ -786,7 +762,7 @@ impl Expr { Self::Stmt(x) => x.iter().all(Stmt::is_pure), - Self::Variable(..) | Self::Stack(..) => true, + Self::Variable(..) => true, _ => self.is_constant(), } @@ -810,8 +786,7 @@ impl Expr { | Self::IntegerConstant(..) | Self::CharConstant(..) | Self::StringConstant(..) - | Self::Unit(..) - | Self::Stack(..) => true, + | Self::Unit(..) => true, Self::InterpolatedString(x, ..) | Self::Array(x, ..) => x.iter().all(Self::is_constant), @@ -872,8 +847,6 @@ impl Expr { Token::LeftParen => true, _ => false, }, - - Self::Stack(..) => false, } } /// Recursively walk this expression. diff --git a/src/eval/chaining.rs b/src/eval/chaining.rs index 8b87e79c..61e400c7 100644 --- a/src/eval/chaining.rs +++ b/src/eval/chaining.rs @@ -684,16 +684,13 @@ impl Engine { Expr::MethodCall(x, ..) if _parent_chain_type == ChainType::Dotting && !x.is_qualified() => { - let crate::ast::FnCallExpr { - args, constants, .. - } = x.as_ref(); + let crate::ast::FnCallExpr { args, .. } = x.as_ref(); let (values, pos) = args.iter().try_fold( (crate::FnArgsVec::with_capacity(args.len()), Position::NONE), |(mut values, mut pos), expr| { - let (value, arg_pos) = self.get_arg_value( - scope, global, state, lib, this_ptr, expr, constants, level, - )?; + let (value, arg_pos) = + self.get_arg_value(scope, global, state, lib, this_ptr, expr, level)?; if values.is_empty() { pos = arg_pos; } @@ -732,15 +729,13 @@ impl Engine { Expr::MethodCall(x, ..) if _parent_chain_type == ChainType::Dotting && !x.is_qualified() => { - let crate::ast::FnCallExpr { - args, constants, .. - } = x.as_ref(); + let crate::ast::FnCallExpr { args, .. } = x.as_ref(); let (values, pos) = args.iter().try_fold( (crate::FnArgsVec::with_capacity(args.len()), Position::NONE), |(mut values, mut pos), expr| { let (value, arg_pos) = self.get_arg_value( - scope, global, state, lib, this_ptr, expr, constants, level, + scope, global, state, lib, this_ptr, expr, level, )?; if values.is_empty() { pos = arg_pos diff --git a/src/eval/expr.rs b/src/eval/expr.rs index b2a155b3..af479490 100644 --- a/src/eval/expr.rs +++ b/src/eval/expr.rs @@ -212,7 +212,6 @@ impl Engine { capture_parent_scope: capture, hashes, args, - constants, .. } = expr; @@ -222,8 +221,7 @@ impl Engine { let hash = hashes.native; return self.make_qualified_function_call( - scope, global, state, lib, this_ptr, namespace, name, args, constants, hash, pos, - level, + scope, global, state, lib, this_ptr, namespace, name, args, hash, pos, level, ); } @@ -234,8 +232,8 @@ impl Engine { ); self.make_function_call( - scope, global, state, lib, this_ptr, name, first_arg, args, constants, *hashes, - *capture, pos, level, + scope, global, state, lib, this_ptr, name, first_arg, args, *hashes, *capture, pos, + level, ) } diff --git a/src/func/call.rs b/src/func/call.rs index dc3090af..37083454 100644 --- a/src/func/call.rs +++ b/src/func/call.rs @@ -958,35 +958,30 @@ impl Engine { lib: &[&Module], this_ptr: &mut Option<&mut Dynamic>, arg_expr: &Expr, - constants: &[Dynamic], level: usize, ) -> RhaiResultOf<(Dynamic, Position)> { - Ok(( - if let Expr::Stack(slot, ..) = arg_expr { + #[cfg(feature = "debugging")] + if self.debugger.is_some() { + if let Some(value) = arg_expr.get_literal_value() { #[cfg(feature = "debugging")] self.run_debugger(scope, global, state, lib, this_ptr, arg_expr, level)?; - constants[*slot].clone() - } else if let Some(value) = arg_expr.get_literal_value() { - #[cfg(feature = "debugging")] - self.run_debugger(scope, global, state, lib, this_ptr, arg_expr, level)?; - value - } else { - // Do not match function exit for arguments - #[cfg(feature = "debugging")] - let reset_debugger = global.debugger.clear_status_if(|status| { - matches!(status, crate::eval::DebuggerStatus::FunctionExit(..)) - }); + return Ok((value, arg_expr.start_position())); + } + } - let result = self.eval_expr(scope, global, state, lib, this_ptr, arg_expr, level); + // Do not match function exit for arguments + #[cfg(feature = "debugging")] + let reset_debugger = global.debugger.clear_status_if(|status| { + matches!(status, crate::eval::DebuggerStatus::FunctionExit(..)) + }); - // Restore function exit status - #[cfg(feature = "debugging")] - global.debugger.reset_status(reset_debugger); + let result = self.eval_expr(scope, global, state, lib, this_ptr, arg_expr, level); - result? - }, - arg_expr.start_position(), - )) + // Restore function exit status + #[cfg(feature = "debugging")] + global.debugger.reset_status(reset_debugger); + + Ok((result?, arg_expr.start_position())) } /// Call a function in normal function-call style. @@ -1000,7 +995,6 @@ impl Engine { fn_name: &str, first_arg: Option<&Expr>, args_expr: &[Expr], - constants: &[Dynamic], hashes: FnCallHashes, capture_scope: bool, pos: Position, @@ -1019,7 +1013,7 @@ impl Engine { KEYWORD_FN_PTR_CALL if total_args >= 1 => { let arg = first_arg.unwrap(); let (arg_value, arg_pos) = - self.get_arg_value(scope, global, state, lib, this_ptr, arg, constants, level)?; + self.get_arg_value(scope, global, state, lib, this_ptr, arg, level)?; if !arg_value.is::() { return Err(self.make_type_mismatch_err::( @@ -1054,7 +1048,7 @@ impl Engine { KEYWORD_FN_PTR if total_args == 1 => { let arg = first_arg.unwrap(); let (arg_value, arg_pos) = - self.get_arg_value(scope, global, state, lib, this_ptr, arg, constants, level)?; + self.get_arg_value(scope, global, state, lib, this_ptr, arg, level)?; // Fn - only in function call style return arg_value @@ -1068,8 +1062,8 @@ impl Engine { // Handle curry() KEYWORD_FN_PTR_CURRY if total_args > 1 => { let first = first_arg.unwrap(); - let (arg_value, arg_pos) = self - .get_arg_value(scope, global, state, lib, this_ptr, first, constants, level)?; + let (arg_value, arg_pos) = + self.get_arg_value(scope, global, state, lib, this_ptr, first, level)?; if !arg_value.is::() { return Err(self.make_type_mismatch_err::( @@ -1082,9 +1076,8 @@ impl Engine { // Append the new curried arguments to the existing list. let fn_curry = a_expr.iter().try_fold(fn_curry, |mut curried, expr| { - let (value, ..) = self.get_arg_value( - scope, global, state, lib, this_ptr, expr, constants, level, - )?; + let (value, ..) = + self.get_arg_value(scope, global, state, lib, this_ptr, expr, level)?; curried.push(value); Ok::<_, RhaiError>(curried) })?; @@ -1097,7 +1090,7 @@ impl Engine { crate::engine::KEYWORD_IS_SHARED if total_args == 1 => { let arg = first_arg.unwrap(); let (arg_value, ..) = - self.get_arg_value(scope, global, state, lib, this_ptr, arg, constants, level)?; + self.get_arg_value(scope, global, state, lib, this_ptr, arg, level)?; return Ok(arg_value.is_shared().into()); } @@ -1105,16 +1098,15 @@ impl Engine { #[cfg(not(feature = "no_function"))] crate::engine::KEYWORD_IS_DEF_FN if total_args == 2 => { let first = first_arg.unwrap(); - let (arg_value, arg_pos) = self - .get_arg_value(scope, global, state, lib, this_ptr, first, constants, level)?; + let (arg_value, arg_pos) = + self.get_arg_value(scope, global, state, lib, this_ptr, first, level)?; let fn_name = arg_value .into_immutable_string() .map_err(|typ| self.make_type_mismatch_err::(typ, arg_pos))?; - let (arg_value, arg_pos) = self.get_arg_value( - scope, global, state, lib, this_ptr, &a_expr[0], constants, level, - )?; + let (arg_value, arg_pos) = + self.get_arg_value(scope, global, state, lib, this_ptr, &a_expr[0], level)?; let num_params = arg_value .as_int() @@ -1133,7 +1125,7 @@ impl Engine { KEYWORD_IS_DEF_VAR if total_args == 1 => { let arg = first_arg.unwrap(); let (arg_value, arg_pos) = - self.get_arg_value(scope, global, state, lib, this_ptr, arg, constants, level)?; + self.get_arg_value(scope, global, state, lib, this_ptr, arg, level)?; let var_name = arg_value .into_immutable_string() .map_err(|typ| self.make_type_mismatch_err::(typ, arg_pos))?; @@ -1146,7 +1138,7 @@ impl Engine { let orig_scope_len = scope.len(); let arg = first_arg.unwrap(); let (arg_value, pos) = - self.get_arg_value(scope, global, state, lib, this_ptr, arg, constants, level)?; + self.get_arg_value(scope, global, state, lib, this_ptr, arg, level)?; let script = &arg_value .into_immutable_string() .map_err(|typ| self.make_type_mismatch_err::(typ, pos))?; @@ -1195,7 +1187,7 @@ impl Engine { .map(|&v| v) .chain(a_expr.iter()) .try_for_each(|expr| { - self.get_arg_value(scope, global, state, lib, this_ptr, expr, constants, level) + self.get_arg_value(scope, global, state, lib, this_ptr, expr, level) .map(|(value, ..)| arg_values.push(value.flatten())) })?; args.extend(curry.iter_mut()); @@ -1227,7 +1219,7 @@ impl Engine { // func(x, ...) -> x.func(...) a_expr.iter().try_for_each(|expr| { - self.get_arg_value(scope, global, state, lib, this_ptr, expr, constants, level) + self.get_arg_value(scope, global, state, lib, this_ptr, expr, level) .map(|(value, ..)| arg_values.push(value.flatten())) })?; @@ -1262,10 +1254,8 @@ impl Engine { .into_iter() .chain(a_expr.iter()) .try_for_each(|expr| { - self.get_arg_value( - scope, global, state, lib, this_ptr, expr, constants, level, - ) - .map(|(value, ..)| arg_values.push(value.flatten())) + self.get_arg_value(scope, global, state, lib, this_ptr, expr, level) + .map(|(value, ..)| arg_values.push(value.flatten())) })?; args.extend(curry.iter_mut()); args.extend(arg_values.iter_mut()); @@ -1290,7 +1280,6 @@ impl Engine { namespace: &crate::module::Namespace, fn_name: &str, args_expr: &[Expr], - constants: &[Dynamic], hash: u64, pos: Position, level: usize, @@ -1313,7 +1302,7 @@ impl Engine { arg_values.push(Dynamic::UNIT); args_expr.iter().skip(1).try_for_each(|expr| { - self.get_arg_value(scope, global, state, lib, this_ptr, expr, constants, level) + self.get_arg_value(scope, global, state, lib, this_ptr, expr, level) .map(|(value, ..)| arg_values.push(value.flatten())) })?; @@ -1344,7 +1333,7 @@ impl Engine { } else { // func(..., ...) or func(mod::x, ...) args_expr.iter().try_for_each(|expr| { - self.get_arg_value(scope, global, state, lib, this_ptr, expr, constants, level) + self.get_arg_value(scope, global, state, lib, this_ptr, expr, level) .map(|(value, ..)| arg_values.push(value.flatten())) })?; args.extend(arg_values.iter_mut()); diff --git a/src/optimizer.rs b/src/optimizer.rs index fc37aebe..2aeb695b 100644 --- a/src/optimizer.rs +++ b/src/optimizer.rs @@ -438,15 +438,7 @@ fn optimize_stmt(stmt: &mut Stmt, state: &mut OptimizerState, preserve_result: b Expr::FnCall(ref mut x2, ..) => { state.set_dirty(); x.0 = Some(OpAssignment::new_from_base(&x2.name)); - - let value = mem::take(&mut x2.args[1]); - - if let Expr::Stack(slot, pos) = value { - x.1.rhs = - Expr::from_dynamic(mem::take(x2.constants.get_mut(slot).unwrap()), pos); - } else { - x.1.rhs = value; - } + x.1.rhs = mem::take(&mut x2.args[1]); } ref expr => unreachable!("Expr::FnCall expected but gets {:?}", expr), } @@ -1063,7 +1055,6 @@ fn optimize_expr(expr: &mut Expr, state: &mut OptimizerState, chaining: bool) { && x.args[0].is_constant() => { let fn_name = match x.args[0] { - Expr::Stack(slot, ..) => x.constants[slot].clone(), Expr::StringConstant(ref s, ..) => s.clone().into(), _ => Dynamic::UNIT }; @@ -1088,11 +1079,7 @@ fn optimize_expr(expr: &mut Expr, state: &mut OptimizerState, chaining: bool) { && x.args.iter().all(Expr::is_constant) // all arguments are constants //&& !is_valid_identifier(x.name.chars()) // cannot be scripted => { - let arg_values = &mut x.args.iter().map(|e| match e { - Expr::Stack(slot, ..) => x.constants[*slot].clone(), - _ => e.get_literal_value().unwrap() - }).collect::>(); - + let arg_values = &mut x.args.iter().map(|e| e.get_literal_value().unwrap()).collect::>(); let arg_types: StaticVec<_> = arg_values.iter().map(Dynamic::type_id).collect(); match x.name.as_str() { @@ -1131,14 +1118,21 @@ fn optimize_expr(expr: &mut Expr, state: &mut OptimizerState, chaining: bool) { x.args.iter_mut().for_each(|a| optimize_expr(a, state, false)); // Move constant arguments - let constants = &mut x.constants; - x.args.iter_mut().for_each(|arg| { - if let Some(value) = arg.get_literal_value() { - state.set_dirty(); - constants.push(value); - *arg = Expr::Stack(constants.len()-1, arg.start_position()); + for arg in x.args.iter_mut() { + match arg { + Expr::DynamicConstant(..) | Expr::Unit(..) + | Expr::StringConstant(..) | Expr::CharConstant(..) + | Expr::BoolConstant(..) | Expr::IntegerConstant(..) => (), + + #[cfg(not(feature = "no_float"))] + Expr:: FloatConstant(..) => (), + + _ => if let Some(value) = arg.get_literal_value() { + state.set_dirty(); + *arg = Expr::DynamicConstant(value.into(), arg.start_position()); + }, } - }); + } } // Eagerly call functions @@ -1154,10 +1148,7 @@ fn optimize_expr(expr: &mut Expr, state: &mut OptimizerState, chaining: bool) { let has_script_fn = false; if !has_script_fn { - let arg_values = &mut x.args.iter().map(|e| match e { - Expr::Stack(slot, ..) => x.constants[*slot].clone(), - _ => e.get_literal_value().unwrap() - }).collect::>(); + let arg_values = &mut x.args.iter().map(|e| e.get_literal_value().unwrap()).collect::>(); let result = match x.name.as_str() { KEYWORD_TYPE_OF if arg_values.len() == 1 => Some(state.engine.map_type_name(arg_values[0].type_name()).into()), @@ -1181,10 +1172,18 @@ fn optimize_expr(expr: &mut Expr, state: &mut OptimizerState, chaining: bool) { optimize_expr(arg, state, false); // Move constant arguments - if let Some(value) = arg.get_literal_value() { - state.set_dirty(); - x.constants.push(value); - *arg = Expr::Stack(x.constants.len()-1, arg.start_position()); + match arg { + Expr::DynamicConstant(..) | Expr::Unit(..) + | Expr::StringConstant(..) | Expr::CharConstant(..) + | Expr::BoolConstant(..) | Expr::IntegerConstant(..) => (), + + #[cfg(not(feature = "no_float"))] + Expr:: FloatConstant(..) => (), + + _ => if let Some(value) = arg.get_literal_value() { + state.set_dirty(); + *arg = Expr::DynamicConstant(value.into(), arg.start_position()); + }, } }, diff --git a/src/parser.rs b/src/parser.rs index f96c2e1c..2a8949d5 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -521,7 +521,6 @@ impl Engine { namespace, hashes, args, - constants: StaticVec::new_const(), pos: settings.pos, } .into_fn_call_expr(settings.pos)); @@ -586,7 +585,6 @@ impl Engine { namespace, hashes, args, - constants: StaticVec::new_const(), pos: settings.pos, } .into_fn_call_expr(settings.pos));