diff --git a/.jules/bolt.md b/.jules/bolt.md new file mode 100644 index 0000000..2d28ad5 --- /dev/null +++ b/.jules/bolt.md @@ -0,0 +1,4 @@ + +## 2024-03-21 - Optimize Context state access and lock contention +**Learning:** Holding a Mutex lock while invoking function pointers in Context can lead to lock contention and blocking threads when context variables are computed by long-running functions. Moreover, multiple map lookups and excessive cloning create micro-bottlenecks. +**Action:** Extract cloned value before invoking the inner function inside the Context value mapping, drop the MutexGuard as early as possible and optimize Hashmap lookups with pattern matching to avoid doing redundant copies of values. diff --git a/benches/display_expression.rs b/benches/display_expression.rs index f9d04d1..5bcd457 100644 --- a/benches/display_expression.rs +++ b/benches/display_expression.rs @@ -1,4 +1,4 @@ -use criterion::{criterion_group, criterion_main, Criterion, black_box}; +use criterion::{black_box, criterion_group, criterion_main, Criterion}; use expression_engine::{parse_expression, ExprAST}; fn bench_display_expression(c: &mut Criterion) { diff --git a/src/context.rs b/src/context.rs index 7d987cb..b61e84e 100644 --- a/src/context.rs +++ b/src/context.rs @@ -33,7 +33,7 @@ impl Context { pub fn get_func(&self, name: &str) -> Option> { let value = self.get(name)?; match value { - ContextValue::Function(func) => Some(func.clone()), + ContextValue::Function(func) => Some(func), ContextValue::Variable(_) => None, } } @@ -41,30 +41,72 @@ impl Context { pub fn get_variable(&self, name: &str) -> Option { let value = self.get(name)?; match value { - ContextValue::Variable(v) => Some(v.clone()), + ContextValue::Variable(v) => Some(v), ContextValue::Function(_) => None, } } pub fn get(&self, name: &str) -> Option { let binding = self.0.lock().unwrap(); - let value = binding.get(name)?; - Some(value.clone()) + binding.get(name).cloned() } pub fn value(&self, name: &str) -> Result { - let binding = self.0.lock().unwrap(); - if binding.get(name).is_none() { - return Ok(Value::None); - } - let value = binding.get(name).unwrap(); + let value = { + let binding = self.0.lock().unwrap(); + binding.get(name).cloned() + }; + match value { - ContextValue::Variable(v) => Ok(v.clone()), - ContextValue::Function(func) => func(Vec::new()), + Some(ContextValue::Variable(v)) => Ok(v), + Some(ContextValue::Function(func)) => func(Vec::new()), + None => Ok(Value::None), } } } +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_context_value_none() { + let ctx = Context::new(); + assert_eq!(ctx.value("missing").unwrap(), Value::None); + } + + #[test] + fn test_context_get_miss() { + let ctx = Context::new(); + assert!(ctx.get("missing").is_none()); + assert!(ctx.get_variable("missing").is_none()); + assert!(ctx.get_func("missing").is_none()); + } + + #[test] + fn test_context_get_hit() { + let mut ctx = Context::new(); + ctx.set_variable("var", Value::from(42)); + ctx.set_func("func", Arc::new(|_| Ok(Value::from(100)))); + + assert!(matches!(ctx.get("var").unwrap(), ContextValue::Variable(_))); + assert_eq!(ctx.get_variable("var").unwrap(), Value::from(42)); + assert!(ctx.get_func("var").is_none()); + + assert!(matches!( + ctx.get("func").unwrap(), + ContextValue::Function(_) + )); + assert!(ctx.get_func("func").is_some()); + assert!(ctx.get_variable("func").is_none()); + + // Also hit value() for the variable case since the earlier test only checked None + assert_eq!(ctx.value("var").unwrap(), Value::from(42)); + // And hit value() for the function case + assert_eq!(ctx.value("func").unwrap(), Value::from(100)); + } +} + /// ///```rust /// use expression_engine::create_context; diff --git a/src/parser.rs b/src/parser.rs index aab9cc0..155d116 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -52,22 +52,16 @@ impl<'a> fmt::Display for ExprAST<'a> { Self::Unary(op, rhs) => { write!(f, "Unary AST: Op: {}, Rhs: {}", op, rhs) } - Self::Binary(op, lhs, rhs) => write!( - f, - "Binary AST: Op: {}, Lhs: {}, Rhs: {}", - op, - lhs, - rhs - ), + Self::Binary(op, lhs, rhs) => { + write!(f, "Binary AST: Op: {}, Lhs: {}, Rhs: {}", op, lhs, rhs) + } Self::Postfix(lhs, op) => { write!(f, "Postfix AST: Lhs: {}, Op: {}", lhs, op,) } Self::Ternary(condition, lhs, rhs) => write!( f, "Ternary AST: Condition: {}, Lhs: {}, Rhs: {}", - condition, - lhs, - rhs + condition, lhs, rhs ), Self::Reference(name) => write!(f, "Reference AST: reference: {}", name), Self::Function(name, params) => {