From 5a1a141ce367d4f78f9cb0b8fe9d1d02fe84f87e Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Sat, 8 Aug 2020 16:24:10 +0800 Subject: [PATCH] Improve shared value treatment. --- .gitignore | 4 +-- src/any.rs | 53 +++++++++++++++++++++++++++------- src/engine.rs | 75 +++++++++++++++++++++++++++--------------------- src/fn_call.rs | 4 +-- src/fn_native.rs | 29 ++++++------------- src/scope.rs | 4 +-- 6 files changed, 100 insertions(+), 69 deletions(-) diff --git a/.gitignore b/.gitignore index e884ce0f..bdd52cb9 100644 --- a/.gitignore +++ b/.gitignore @@ -3,5 +3,5 @@ Cargo.lock .vscode/ .cargo/ doc/book/ -before -after +before* +after* diff --git a/src/any.rs b/src/any.rs index 7ea7436c..5d47465d 100644 --- a/src/any.rs +++ b/src/any.rs @@ -5,7 +5,7 @@ use crate::parser::{ImmutableString, INT}; use crate::r#unsafe::{unsafe_cast_box, unsafe_try_cast}; #[cfg(not(feature = "no_closure"))] -use crate::fn_native::SharedMut; +use crate::fn_native::{shared_try_take, Shared}; #[cfg(not(feature = "no_float"))] use crate::parser::FLOAT; @@ -159,9 +159,15 @@ pub enum Union { #[cfg(not(feature = "no_object"))] Map(Box), FnPtr(Box), + Variant(Box>), + #[cfg(not(feature = "no_closure"))] - Shared(SharedMut), + #[cfg(not(feature = "sync"))] + Shared(Shared>), + #[cfg(not(feature = "no_closure"))] + #[cfg(feature = "sync")] + Shared(Shared>), } /// Underlying `Variant` read guard for `Dynamic`. @@ -176,6 +182,7 @@ pub struct DynamicReadLock<'d, T: Variant + Clone>(DynamicReadLockInner<'d, T>); enum DynamicReadLockInner<'d, T: Variant + Clone> { /// A simple reference to a non-shared value. Reference(&'d T), + /// A read guard to a shared `RefCell`. #[cfg(not(feature = "no_closure"))] #[cfg(not(feature = "sync"))] @@ -212,6 +219,7 @@ pub struct DynamicWriteLock<'d, T: Variant + Clone>(DynamicWriteLockInner<'d, T> enum DynamicWriteLockInner<'d, T: Variant + Clone> { /// A simple mutable reference to a non-shared value. Reference(&'d mut T), + /// A write guard to a shared `RefCell`. #[cfg(not(feature = "no_closure"))] #[cfg(not(feature = "sync"))] @@ -406,6 +414,7 @@ impl fmt::Display for Dynamic { #[cfg(not(feature = "no_std"))] Union::Variant(value) if value.is::() => f.write_str(""), Union::Variant(value) => f.write_str((*value).type_name()), + #[cfg(not(feature = "no_closure"))] #[cfg(not(feature = "sync"))] Union::Shared(cell) => { @@ -444,6 +453,7 @@ impl fmt::Debug for Dynamic { #[cfg(not(feature = "no_std"))] Union::Variant(value) if value.is::() => write!(f, ""), Union::Variant(value) => write!(f, "{}", (*value).type_name()), + #[cfg(not(feature = "no_closure"))] #[cfg(not(feature = "sync"))] Union::Shared(cell) => { @@ -779,25 +789,46 @@ impl Dynamic { self.try_cast::().unwrap() } - /// Get a copy of the `Dynamic` as a specific type. + /// Dereference the `Dynamic`. /// - /// If the `Dynamic` is not a shared value, it returns a cloned copy of the value. + /// If the `Dynamic` is not a shared value, it returns a cloned copy. /// - /// If the `Dynamic` is a shared value, it returns a cloned copy of the shared value. - /// - /// Returns `None` if the cast fails. + /// If the `Dynamic` is a shared value, it a cloned copy of the shared value. #[inline(always)] - pub fn clone_inner_data(self) -> Option { + pub fn get_inner_clone(&self) -> Self { + match &self.0 { + #[cfg(not(feature = "no_closure"))] + Union::Shared(cell) => { + #[cfg(not(feature = "sync"))] + return cell.borrow().clone(); + + #[cfg(feature = "sync")] + return cell.read().unwrap().clone(); + } + _ => self.clone(), + } + } + + /// Flatten the `Dynamic`. + /// + /// If the `Dynamic` is not a shared value, it returns itself. + /// + /// If the `Dynamic` is a shared value, it returns the shared value if there are + /// no outstanding references, or a cloned copy. + #[inline(always)] + pub fn flatten(self) -> Self { match self.0 { #[cfg(not(feature = "no_closure"))] Union::Shared(cell) => { #[cfg(not(feature = "sync"))] - return Some(cell.borrow().downcast_ref::().unwrap().clone()); + return shared_try_take(cell) + .map_or_else(|c| c.borrow().clone(), RefCell::into_inner); #[cfg(feature = "sync")] - return Some(cell.read().unwrap().downcast_ref::().unwrap().clone()); + return shared_try_take(cell) + .map_or_else(|c| c.read().unwrap().clone(), RwLock::into_inner); } - _ => self.try_cast(), + _ => self, } } diff --git a/src/engine.rs b/src/engine.rs index 9a771ac6..5c4fdaee 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -147,6 +147,7 @@ pub enum Target<'a> { #[cfg(any(not(feature = "no_index"), not(feature = "no_object")))] impl Target<'_> { /// Is the `Target` a reference pointing to other data? + #[inline(always)] pub fn is_ref(&self) -> bool { match self { Self::Ref(_) => true, @@ -159,6 +160,7 @@ impl Target<'_> { } } /// Is the `Target` an owned value? + #[inline(always)] pub fn is_value(&self) -> bool { match self { Self::Ref(_) => false, @@ -171,6 +173,7 @@ impl Target<'_> { } } /// Is the `Target` a shared value? + #[inline(always)] pub fn is_shared(&self) -> bool { match self { Self::Ref(r) => r.is_shared(), @@ -184,6 +187,7 @@ impl Target<'_> { } /// Is the `Target` a specific type? #[allow(dead_code)] + #[inline(always)] pub fn is(&self) -> bool { match self { Target::Ref(r) => r.is::(), @@ -196,6 +200,7 @@ impl Target<'_> { } } /// Get the value of the `Target` as a `Dynamic`, cloning a referenced value if necessary. + #[inline(always)] pub fn clone_into_dynamic(self) -> Dynamic { match self { Self::Ref(r) => r.clone(), // Referenced value is cloned @@ -208,6 +213,7 @@ impl Target<'_> { } } /// Get a mutable reference from the `Target`. + #[inline(always)] pub fn as_mut(&mut self) -> &mut Dynamic { match self { Self::Ref(r) => *r, @@ -258,6 +264,7 @@ impl Target<'_> { #[cfg(any(not(feature = "no_index"), not(feature = "no_object")))] impl<'a> From<&'a mut Dynamic> for Target<'a> { + #[inline(always)] fn from(value: &'a mut Dynamic) -> Self { #[cfg(not(feature = "no_closure"))] #[cfg(not(feature = "no_object"))] @@ -273,6 +280,7 @@ impl<'a> From<&'a mut Dynamic> for Target<'a> { #[cfg(any(not(feature = "no_index"), not(feature = "no_object")))] impl> From for Target<'_> { + #[inline(always)] fn from(value: T) -> Self { Self::Value(value.into()) } @@ -301,6 +309,7 @@ pub struct State { impl State { /// Create a new `State`. + #[inline(always)] pub fn new() -> Self { Default::default() } @@ -407,6 +416,7 @@ pub struct Engine { } impl fmt::Debug for Engine { + #[inline(always)] fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self.id.as_ref() { Some(id) => write!(f, "Engine({})", id), @@ -689,7 +699,7 @@ impl Engine { idx_values: &mut StaticVec, chain_type: ChainType, level: usize, - mut _new_val: Option, + new_val: Option, ) -> Result<(Dynamic, bool), Box> { if chain_type == ChainType::None { panic!(); @@ -722,12 +732,12 @@ impl Engine { self.eval_dot_index_chain_helper( state, lib, this_ptr, obj_ptr, expr, idx_values, next_chain, level, - _new_val, + new_val, ) .map_err(|err| err.new_position(*pos)) } // xxx[rhs] = new_val - _ if _new_val.is_some() => { + _ if new_val.is_some() => { let mut idx_val2 = idx_val.clone(); // `call_setter` is introduced to bypass double mutable borrowing of target @@ -737,7 +747,7 @@ impl Engine { // Indexed value is a reference - update directly Ok(ref mut obj_ptr) => { obj_ptr - .set_value(_new_val.unwrap()) + .set_value(new_val.unwrap()) .map_err(|err| err.new_position(rhs.position()))?; None @@ -747,7 +757,7 @@ impl Engine { #[cfg(not(feature = "no_index"))] EvalAltResult::ErrorIndexingType(_, _) => { // Raise error if there is no index getter nor setter - Some(_new_val.unwrap()) + Some(new_val.unwrap()) } // Any other error - return err => return Err(Box::new(err)), @@ -799,13 +809,13 @@ impl Engine { // xxx.module::fn_name(...) - syntax error Expr::FnCall(_) => unreachable!(), // {xxx:map}.id = ??? - Expr::Property(x) if target.is::() && _new_val.is_some() => { + Expr::Property(x) if target.is::() && new_val.is_some() => { let ((prop, _, _), pos) = x.as_ref(); let index = prop.clone().into(); let mut val = self .get_indexed_mut(state, lib, target, index, *pos, true, false, level)?; - val.set_value(_new_val.unwrap()) + val.set_value(new_val.unwrap()) .map_err(|err| err.new_position(rhs.position()))?; Ok((Default::default(), true)) } @@ -820,9 +830,10 @@ impl Engine { Ok((val.clone_into_dynamic(), false)) } // xxx.id = ??? - Expr::Property(x) if _new_val.is_some() => { + Expr::Property(x) if new_val.is_some() => { let ((_, _, setter), pos) = x.as_ref(); - let mut args = [target.as_mut(), _new_val.as_mut().unwrap()]; + let mut new_val = new_val; + let mut args = [target.as_mut(), new_val.as_mut().unwrap()]; self.exec_fn_call( state, lib, setter, 0, &mut args, is_ref, true, false, None, None, level, @@ -872,7 +883,7 @@ impl Engine { self.eval_dot_index_chain_helper( state, lib, this_ptr, &mut val, expr, idx_values, next_chain, level, - _new_val, + new_val, ) .map_err(|err| err.new_position(*pos)) } @@ -905,7 +916,7 @@ impl Engine { idx_values, next_chain, level, - _new_val, + new_val, ) .map_err(|err| err.new_position(*pos))?; @@ -944,7 +955,7 @@ impl Engine { self.eval_dot_index_chain_helper( state, lib, this_ptr, target, expr, idx_values, next_chain, - level, _new_val, + level, new_val, ) .map_err(|err| err.new_position(*pos)) } @@ -1114,7 +1125,7 @@ impl Engine { state: &mut State, _lib: &Module, target: &'a mut Target, - mut _idx: Dynamic, + idx: Dynamic, idx_pos: Position, _create: bool, _indexers: bool, @@ -1132,7 +1143,7 @@ impl Engine { #[cfg(not(feature = "no_index"))] Dynamic(Union::Array(arr)) => { // val_array[idx] - let index = _idx + let index = idx .as_int() .map_err(|_| EvalAltResult::ErrorNumericIndexExpr(idx_pos))?; @@ -1153,13 +1164,13 @@ impl Engine { Dynamic(Union::Map(map)) => { // val_map[idx] Ok(if _create { - let index = _idx + let index = idx .take_immutable_string() .map_err(|_| EvalAltResult::ErrorStringIndexExpr(idx_pos))?; map.entry(index).or_insert(Default::default()).into() } else { - let index = _idx + let index = idx .read_lock::() .ok_or_else(|| EvalAltResult::ErrorStringIndexExpr(idx_pos))?; @@ -1173,7 +1184,7 @@ impl Engine { Dynamic(Union::Str(s)) => { // val_string[idx] let chars_len = s.chars().count(); - let index = _idx + let index = idx .as_int() .map_err(|_| EvalAltResult::ErrorNumericIndexExpr(idx_pos))?; @@ -1192,7 +1203,8 @@ impl Engine { #[cfg(not(feature = "no_index"))] _ if _indexers => { let type_name = val.type_name(); - let args = &mut [val, &mut _idx]; + let mut idx = idx; + let args = &mut [val, &mut idx]; self.exec_fn_call( state, _lib, FN_IDX_GET, 0, args, is_ref, true, false, None, None, _level, ) @@ -1331,11 +1343,11 @@ impl Engine { )), // Normal assignment ScopeEntryType::Normal if op.is_empty() => { - let rhs_val = rhs_val.clone_inner_data().unwrap(); + let value = rhs_val.flatten(); if cfg!(not(feature = "no_closure")) && lhs_ptr.is_shared() { - *lhs_ptr.write_lock::().unwrap() = rhs_val; + *lhs_ptr.write_lock::().unwrap() = value; } else { - *lhs_ptr = rhs_val; + *lhs_ptr = value; } Ok(Default::default()) } @@ -1378,7 +1390,7 @@ impl Engine { ) .map_err(|err| err.new_position(*op_pos))?; - let value = value.clone_inner_data().unwrap(); + let value = value.flatten(); if cfg!(not(feature = "no_closure")) && lhs_ptr.is_shared() { *lhs_ptr.write_lock::().unwrap() = value; } else { @@ -1674,14 +1686,14 @@ impl Engine { let index = scope.len() - 1; state.scope_level += 1; - for loop_var in func(iter_type) { - let for_var = scope.get_mut(index).0; - let value = loop_var.clone_inner_data().unwrap(); + for iter_value in func(iter_type) { + let (loop_var, _) = scope.get_mut(index); - if cfg!(not(feature = "no_closure")) && for_var.is_shared() { - *for_var.write_lock().unwrap() = value; + let value = iter_value.flatten(); + if cfg!(not(feature = "no_closure")) && loop_var.is_shared() { + *loop_var.write_lock().unwrap() = value; } else { - *for_var = value; + *loop_var = value; } self.inc_operations(state) @@ -1750,8 +1762,7 @@ impl Engine { let expr = expr.as_ref().unwrap(); let val = self .eval_expr(scope, mods, state, lib, this_ptr, expr, level)? - .clone_inner_data() - .unwrap(); + .flatten(); let var_name = unsafe_cast_var_name_to_lifetime(var_name, &state); scope.push_dynamic_value(var_name, ScopeEntryType::Normal, val, false); Ok(Default::default()) @@ -1769,8 +1780,7 @@ impl Engine { let ((var_name, _), expr, _) = x.as_ref(); let val = self .eval_expr(scope, mods, state, lib, this_ptr, &expr, level)? - .clone_inner_data() - .unwrap(); + .flatten(); let var_name = unsafe_cast_var_name_to_lifetime(var_name, &state); scope.push_dynamic_value(var_name, ScopeEntryType::Constant, val, true); Ok(Default::default()) @@ -2000,6 +2010,7 @@ impl Engine { } /// Map a type_name into a pretty-print name + #[inline(always)] pub(crate) fn map_type_name<'a>(&'a self, name: &'a str) -> &'a str { self.type_names .as_ref() diff --git a/src/fn_call.rs b/src/fn_call.rs index 20557748..87cc4e2b 100644 --- a/src/fn_call.rs +++ b/src/fn_call.rs @@ -925,11 +925,11 @@ impl Engine { self.inc_operations(state) .map_err(|err| err.new_position(pos))?; - // Turn it into a method call only if the object is not shared args = if target.is_shared() { - arg_values.insert(0, target.clone().clone_inner_data().unwrap()); + arg_values.insert(0, target.get_inner_clone()); arg_values.iter_mut().collect() } else { + // Turn it into a method call only if the object is not shared is_ref = true; once(target).chain(arg_values.iter_mut()).collect() }; diff --git a/src/fn_native.rs b/src/fn_native.rs index cc3ed0dc..a32e7d8b 100644 --- a/src/fn_native.rs +++ b/src/fn_native.rs @@ -21,13 +21,6 @@ use crate::stdlib::rc::Rc; #[cfg(feature = "sync")] use crate::stdlib::sync::Arc; -#[cfg(not(feature = "no_closure"))] -#[cfg(not(feature = "sync"))] -use crate::stdlib::cell::RefCell; -#[cfg(not(feature = "no_closure"))] -#[cfg(feature = "sync")] -use crate::stdlib::sync::RwLock; - /// Trait that maps to `Send + Sync` only under the `sync` feature. #[cfg(feature = "sync")] pub trait SendSync: Send + Sync {} @@ -49,15 +42,6 @@ pub type Shared = Rc; #[cfg(feature = "sync")] pub type Shared = Arc; -/// Mutable reference-counted container (read-write lock) -#[cfg(not(feature = "no_closure"))] -#[cfg(not(feature = "sync"))] -pub type SharedMut = Shared>; -/// Mutable reference-counted container (read-write lock) -#[cfg(not(feature = "no_closure"))] -#[cfg(feature = "sync")] -pub type SharedMut = Shared>; - /// Consume a `Shared` resource and return a mutable reference to the wrapped value. /// If the resource is shared (i.e. has other outstanding references), a cloned copy is used. pub fn shared_make_mut(value: &mut Shared) -> &mut T { @@ -67,16 +51,21 @@ pub fn shared_make_mut(value: &mut Shared) -> &mut T { return Arc::make_mut(value); } +/// Consume a `Shared` resource if is unique (i.e. not shared). +pub fn shared_try_take(value: Shared) -> Result> { + #[cfg(not(feature = "sync"))] + return Rc::try_unwrap(value); + #[cfg(feature = "sync")] + return Arc::try_unwrap(value); +} + /// Consume a `Shared` resource, assuming that it is unique (i.e. not shared). /// /// # Panics /// /// Panics if the resource is shared (i.e. has other outstanding references). pub fn shared_take(value: Shared) -> T { - #[cfg(not(feature = "sync"))] - return Rc::try_unwrap(value).map_err(|_| ()).unwrap(); - #[cfg(feature = "sync")] - return Arc::try_unwrap(value).map_err(|_| ()).unwrap(); + shared_try_take(value).map_err(|_| ()).unwrap() } pub type FnCallArgs<'a> = [&'a mut Dynamic]; diff --git a/src/scope.rs b/src/scope.rs index 61f6fa17..afc55e72 100644 --- a/src/scope.rs +++ b/src/scope.rs @@ -338,7 +338,7 @@ impl<'a> Scope<'a> { /// ``` pub fn get_value(&self, name: &str) -> Option { self.get_entry(name) - .and_then(|Entry { value, .. }| value.clone().clone_inner_data::()) + .and_then(|Entry { value, .. }| value.get_inner_clone().try_cast()) } /// Update the value of the named entry. @@ -441,7 +441,7 @@ impl<'a> Scope<'a> { /// ``` pub fn iter(&self) -> impl Iterator { self.iter_raw() - .map(|(name, value)| (name, value.clone().clone_inner_data().unwrap())) + .map(|(name, value)| (name, value.get_inner_clone())) } /// Get an iterator to entries in the Scope.