From 0d7f2c16cc869e25d619b5cf3185f3ad342e5930 Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Tue, 23 Mar 2021 20:04:54 +0800 Subject: [PATCH] Reduce indirections. --- CHANGELOG.md | 20 +++--- src/ast.rs | 2 +- src/engine.rs | 55 +++++++------- src/engine_api.rs | 4 +- src/parser.rs | 5 +- src/token.rs | 178 +++++++++++++++++++++++++--------------------- 6 files changed, 137 insertions(+), 127 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ad8a65cf..72117b4a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,21 +4,23 @@ Rhai Release Notes Version 0.19.15 =============== -This version replaces internal usage of `HashMap` with `BTreeMap` in many cases, which should result -in speed improvements because a `BTreeMap` is faster when the number of items held is small. +This version replaces all internal usage of `HashMap` with `BTreeMap`, which should result +in some speed improvement because a `BTreeMap` is leaner when the number of items held is small. +Most, if not all, collections in Rhai hold very few data items, so this is a typical scenario of +many tiny-sized collections. -This also translates to the Rhai object map type, `Map`, which used to be an alias to `HashMap` and -is now aliased to `BTreeMap` instead. This change is due to the fact that, in the vast majority of -object map usages, the number of properties held is small. +The Rhai object map type, `Map`, used to be an alias to `HashMap` and is now aliased to `BTreeMap` +instead. This is also because, in the vast majority of usage cases, the number of properties held by +an object map is small. -`HashMap` and `BTreeMap` have almost identical public API's so this change is unlikely to break a -lot of existing code. +`HashMap` and `BTreeMap` have almost identical public API's so this change is unlikely to break +existing code. Breaking changes ---------------- -* `Map` is now an alias to `BTreeMap` instead of `HashMap`. This is because most object maps used have few properties. +* `Map` is now an alias to `BTreeMap` instead of `HashMap` because most object maps hold few properties. * The traits `RegisterFn` and `RegisterResultFn` are removed. `Engine::register_fn` and `Engine::register_result_fn` are now implemented directly on `Engine`. * `FnPtr::call_dynamic` now takes `&NativeCallContext` instead of consuming it. * All `Module::set_fn_XXX` methods are removed, in favor of `Module::set_native_fn`. @@ -28,7 +30,7 @@ Breaking changes Enhancements ------------ -* Replaced most `HashMap` usage with `BTreeMap` for better performance when the number of items is small. +* Replaced all `HashMap` usage with `BTreeMap` for better performance because collections in Rhai are tiny. * `Engine::register_result_fn` no longer requires the successful return type to be `Dynamic`. It can now be any clonable type. * `#[rhai_fn(return_raw)]` can now return `Result>` where `T` is any clonable type instead of `Result>`. diff --git a/src/ast.rs b/src/ast.rs index 3651db11..5934ff34 100644 --- a/src/ast.rs +++ b/src/ast.rs @@ -1285,7 +1285,7 @@ pub struct BinaryExpr { pub struct OpAssignment { pub hash_op_assign: u64, pub hash_op: u64, - pub op: Cow<'static, str>, + pub op: &'static str, } /// _(INTERNALS)_ An set of function call hashes. diff --git a/src/engine.rs b/src/engine.rs index a0864b8e..5d9cba21 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -342,13 +342,13 @@ impl<'a> Target<'a> { #[inline(always)] pub fn is(&self) -> bool { match self { - Target::Ref(r) => r.is::(), + Self::Ref(r) => r.is::(), #[cfg(not(feature = "no_closure"))] #[cfg(not(feature = "no_object"))] - Target::LockGuard((r, _)) => r.is::(), - Target::Value(r) => r.is::(), + Self::LockGuard((r, _)) => r.is::(), + Self::Value(r) => r.is::(), #[cfg(not(feature = "no_index"))] - Target::StringChar(_, _, _) => TypeId::of::() == TypeId::of::(), + Self::StringChar(_, _, _) => TypeId::of::() == TypeId::of::(), } } /// Get the value of the `Target` as a `Dynamic`, cloning a referenced value if necessary. @@ -1056,7 +1056,7 @@ impl Engine { idx_values: &mut StaticVec, chain_type: ChainType, level: usize, - new_val: Option<((Dynamic, Position), (&Option, Position))>, + new_val: Option<((Dynamic, Position), (Option, Position))>, ) -> Result<(Dynamic, bool), Box> { assert!(chain_type != ChainType::NonChaining); @@ -1097,6 +1097,8 @@ impl Engine { // xxx[rhs] op= new_val _ if new_val.is_some() => { let idx_val = idx_val.as_index_value(); + + #[cfg(not(feature = "no_index"))] let mut idx_val2 = idx_val.clone(); // `call_setter` is introduced to bypass double mutable borrowing of target @@ -1357,7 +1359,7 @@ impl Engine { this_ptr: &mut Option<&mut Dynamic>, expr: &Expr, level: usize, - new_val: Option<((Dynamic, Position), (&Option, Position))>, + new_val: Option<((Dynamic, Position), (Option, Position))>, ) -> RhaiResult { let (crate::ast::BinaryExpr { lhs, rhs }, chain_type, op_pos) = match expr { Expr::Index(x, pos) => (x.as_ref(), ChainType::Index, *pos), @@ -1524,7 +1526,7 @@ impl Engine { state: &mut State, _lib: &[&Module], target: &'t mut Dynamic, - idx: Dynamic, + mut idx: Dynamic, idx_pos: Position, _create: bool, _is_ref: bool, @@ -1557,21 +1559,18 @@ impl Engine { #[cfg(not(feature = "no_object"))] Dynamic(Union::Map(map, _)) => { // val_map[idx] - Ok(if _create { - let index = idx.take_immutable_string().map_err(|err| { - self.make_type_mismatch_err::(err, idx_pos) - })?; + let index = &*idx.read_lock::().ok_or_else(|| { + self.make_type_mismatch_err::(idx.type_name(), idx_pos) + })?; - map.entry(index).or_insert_with(Default::default).into() - } else { - let index = idx.read_lock::().ok_or_else(|| { - self.make_type_mismatch_err::("", idx_pos) - })?; + if _create && !map.contains_key(index) { + map.insert(index.clone(), Default::default()); + } - map.get_mut(&*index) - .map(Target::from) - .unwrap_or_else(|| Target::from(())) - }) + Ok(map + .get_mut(index) + .map(Target::from) + .unwrap_or_else(|| Target::from(()))) } #[cfg(not(feature = "no_index"))] @@ -1596,7 +1595,6 @@ impl Engine { #[cfg(not(feature = "no_index"))] _ if _indexers => { let type_name = target.type_name(); - let mut idx = idx; let args = &mut [target, &mut idx]; let hash_get = FnCallHash::from_native(calc_fn_hash(empty(), FN_IDX_GET, 2)); self.exec_fn_call( @@ -1863,7 +1861,7 @@ impl Engine { mods: &mut Imports, state: &mut State, lib: &[&Module], - op_info: &Option, + op_info: Option, op_pos: Position, mut target: Target, mut new_value: Dynamic, @@ -1889,20 +1887,19 @@ impl Engine { lhs_ptr_inner = target.as_mut(); } - let hash = *hash_op_assign; + let hash = hash_op_assign; let args = &mut [lhs_ptr_inner, &mut new_value]; match self.call_native_fn(mods, state, lib, op, hash, args, true, true, op_pos) { Ok(_) => (), - Err(err) if matches!(err.as_ref(), EvalAltResult::ErrorFunctionNotFound(f, _) if f.starts_with(op.as_ref())) => + Err(err) if matches!(err.as_ref(), EvalAltResult::ErrorFunctionNotFound(f, _) if f.starts_with(op)) => { // Expand to `var = var op rhs` let op = &op[..op.len() - 1]; // extract operator without = // Run function - let (value, _) = self.call_native_fn( - mods, state, lib, op, *hash_op, args, true, false, op_pos, - )?; + let (value, _) = self + .call_native_fn(mods, state, lib, op, hash_op, args, true, false, op_pos)?; *args[0] = value.flatten(); } @@ -1975,7 +1972,7 @@ impl Engine { mods, state, lib, - op_info, + op_info.clone(), *op_pos, lhs_ptr, rhs_val, @@ -1991,7 +1988,7 @@ impl Engine { let rhs_val = self .eval_expr(scope, mods, state, lib, this_ptr, rhs_expr, level)? .flatten(); - let _new_val = Some(((rhs_val, rhs_expr.position()), (op_info, *op_pos))); + let _new_val = Some(((rhs_val, rhs_expr.position()), (op_info.clone(), *op_pos))); // Must be either `var[index] op= val` or `var.prop op= val` match lhs_expr { diff --git a/src/engine_api.rs b/src/engine_api.rs index 86c523d7..36acc9ff 100644 --- a/src/engine_api.rs +++ b/src/engine_api.rs @@ -1278,9 +1278,9 @@ impl Engine { // Trims the JSON string and add a '#' in front let json_text = json.trim_start(); - let scripts = if json_text.starts_with(Token::MapStart.syntax().as_ref()) { + let scripts = if json_text.starts_with(Token::MapStart.keyword_syntax()) { [json_text, ""] - } else if json_text.starts_with(Token::LeftBrace.syntax().as_ref()) { + } else if json_text.starts_with(Token::LeftBrace.keyword_syntax()) { ["#", json_text] } else { return Err(crate::ParseErrorType::MissingToken( diff --git a/src/parser.rs b/src/parser.rs index c4ab18cb..34f066b2 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -10,7 +10,6 @@ use crate::module::NamespaceRef; use crate::optimize::optimize_into_ast; use crate::optimize::OptimizationLevel; use crate::stdlib::{ - borrow::Cow, boxed::Box, collections::BTreeMap, format, @@ -1355,7 +1354,7 @@ fn parse_unary( /// Make an assignment statement. fn make_assignment_stmt<'a>( - op: Cow<'static, str>, + op: &'static str, state: &mut ParseState, lhs: Expr, rhs: Expr, @@ -1479,7 +1478,7 @@ fn parse_op_assignment_stmt( | Token::PowerOfAssign | Token::AndAssign | Token::OrAssign - | Token::XOrAssign => token.syntax(), + | Token::XOrAssign => token.keyword_syntax(), _ => return Ok(Stmt::Expr(lhs)), }; diff --git a/src/token.rs b/src/token.rs index 4c697381..dba17351 100644 --- a/src/token.rs +++ b/src/token.rs @@ -381,6 +381,99 @@ pub enum Token { } impl Token { + /// Get the syntax of the token if it is a keyword. + /// + /// # Panics + /// + /// Panics if the token is not a keyword. + pub fn keyword_syntax(&self) -> &'static str { + use Token::*; + + match self { + LeftBrace => "{", + RightBrace => "}", + LeftParen => "(", + RightParen => ")", + LeftBracket => "[", + RightBracket => "]", + Plus => "+", + UnaryPlus => "+", + Minus => "-", + UnaryMinus => "-", + Multiply => "*", + Divide => "/", + SemiColon => ";", + Colon => ":", + DoubleColon => "::", + DoubleArrow => "=>", + Underscore => "_", + Comma => ",", + Period => ".", + MapStart => "#{", + Equals => "=", + True => "true", + False => "false", + Let => "let", + Const => "const", + If => "if", + Else => "else", + Switch => "switch", + Do => "do", + While => "while", + Until => "until", + Loop => "loop", + For => "for", + In => "in", + LessThan => "<", + GreaterThan => ">", + Bang => "!", + LessThanEqualsTo => "<=", + GreaterThanEqualsTo => ">=", + EqualsTo => "==", + NotEqualsTo => "!=", + Pipe => "|", + Or => "||", + Ampersand => "&", + And => "&&", + Continue => "continue", + Break => "break", + Return => "return", + Throw => "throw", + Try => "try", + Catch => "catch", + PlusAssign => "+=", + MinusAssign => "-=", + MultiplyAssign => "*=", + DivideAssign => "/=", + LeftShiftAssign => "<<=", + RightShiftAssign => ">>=", + AndAssign => "&=", + OrAssign => "|=", + XOrAssign => "^=", + LeftShift => "<<", + RightShift => ">>", + XOr => "^", + Modulo => "%", + ModuloAssign => "%=", + PowerOf => "**", + PowerOfAssign => "**=", + + #[cfg(not(feature = "no_function"))] + Fn => "fn", + #[cfg(not(feature = "no_function"))] + Private => "private", + + #[cfg(not(feature = "no_module"))] + Import => "import", + #[cfg(not(feature = "no_module"))] + Export => "export", + #[cfg(not(feature = "no_module"))] + As => "as", + + t => unreachable!("{:?} is not a keyword", t), + } + } + /// Get the syntax of the token. pub fn syntax(&self) -> Cow<'static, str> { use Token::*; @@ -399,90 +492,9 @@ impl Token { LexError(err) => err.to_string().into(), Comment(s) => s.clone().into(), - token => match token { - LeftBrace => "{", - RightBrace => "}", - LeftParen => "(", - RightParen => ")", - LeftBracket => "[", - RightBracket => "]", - Plus => "+", - UnaryPlus => "+", - Minus => "-", - UnaryMinus => "-", - Multiply => "*", - Divide => "/", - SemiColon => ";", - Colon => ":", - DoubleColon => "::", - DoubleArrow => "=>", - Underscore => "_", - Comma => ",", - Period => ".", - MapStart => "#{", - Equals => "=", - True => "true", - False => "false", - Let => "let", - Const => "const", - If => "if", - Else => "else", - Switch => "switch", - Do => "do", - While => "while", - Until => "until", - Loop => "loop", - For => "for", - In => "in", - LessThan => "<", - GreaterThan => ">", - Bang => "!", - LessThanEqualsTo => "<=", - GreaterThanEqualsTo => ">=", - EqualsTo => "==", - NotEqualsTo => "!=", - Pipe => "|", - Or => "||", - Ampersand => "&", - And => "&&", - Continue => "continue", - Break => "break", - Return => "return", - Throw => "throw", - Try => "try", - Catch => "catch", - PlusAssign => "+=", - MinusAssign => "-=", - MultiplyAssign => "*=", - DivideAssign => "/=", - LeftShiftAssign => "<<=", - RightShiftAssign => ">>=", - AndAssign => "&=", - OrAssign => "|=", - XOrAssign => "^=", - LeftShift => "<<", - RightShift => ">>", - XOr => "^", - Modulo => "%", - ModuloAssign => "%=", - PowerOf => "**", - PowerOfAssign => "**=", + EOF => "{EOF}".into(), - #[cfg(not(feature = "no_function"))] - Fn => "fn", - #[cfg(not(feature = "no_function"))] - Private => "private", - - #[cfg(not(feature = "no_module"))] - Import => "import", - #[cfg(not(feature = "no_module"))] - Export => "export", - #[cfg(not(feature = "no_module"))] - As => "as", - EOF => "{EOF}", - t => unreachable!("operator should be matched in outer scope: {:?}", t), - } - .into(), + token => token.keyword_syntax().into(), } }