diff --git a/.jules/bolt.md b/.jules/bolt.md new file mode 100644 index 0000000..05dd546 --- /dev/null +++ b/.jules/bolt.md @@ -0,0 +1,3 @@ +## 2024-05-24 - Context Map Lookups and Locking Contention +**Learning:** Double lookups inside `MutexGuard` sections combined with unnecessary entire Enum cloning create notable lock contention and memory allocation overhead. Specifically, `binding.get(name).is_none()` followed by `binding.get(name).unwrap()` traverses the map twice unnecessarily. +**Action:** Always perform a single `.get(name)` map lookup, and match on the borrowed enum reference to clone ONLY the internal value (like an `Arc` or `Value`), bypassing full `ContextValue` copying. Drop the `MutexGuard` explicitly BEFORE invoking external closures/functions (like `func(Vec::new())`) to reduce contention and prevent deadlocks on reentrant code. \ No newline at end of file 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..54fd103 100644 --- a/src/context.rs +++ b/src/context.rs @@ -31,7 +31,8 @@ impl Context { } pub fn get_func(&self, name: &str) -> Option> { - let value = self.get(name)?; + let binding = self.0.lock().unwrap(); + let value = binding.get(name)?; match value { ContextValue::Function(func) => Some(func.clone()), ContextValue::Variable(_) => None, @@ -39,7 +40,8 @@ impl Context { } pub fn get_variable(&self, name: &str) -> Option { - let value = self.get(name)?; + let binding = self.0.lock().unwrap(); + let value = binding.get(name)?; match value { ContextValue::Variable(v) => Some(v.clone()), ContextValue::Function(_) => None, @@ -53,15 +55,15 @@ impl Context { } 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(); - match value { - ContextValue::Variable(v) => Ok(v.clone()), - ContextValue::Function(func) => func(Vec::new()), - } + let func = { + let binding = self.0.lock().unwrap(); + match binding.get(name) { + Some(ContextValue::Variable(v)) => return Ok(v.clone()), + Some(ContextValue::Function(func)) => func.clone(), + None => return Ok(Value::None), + } + }; + func(Vec::new()) } } @@ -104,3 +106,38 @@ macro_rules! create_context { ctx }}; } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_context_lookups() { + let mut ctx = Context::new(); + ctx.set_variable("my_var", Value::from(42)); + ctx.set_func("my_func", Arc::new(|_| Ok(Value::from(100)))); + + // get_variable + assert_eq!(ctx.get_variable("my_var"), Some(Value::from(42))); + assert_eq!(ctx.get_variable("my_func"), None); + assert_eq!(ctx.get_variable("non_existent"), None); + + // get_func + assert!(ctx.get_func("my_func").is_some()); + assert!(ctx.get_func("my_var").is_none()); + assert!(ctx.get_func("non_existent").is_none()); + + // value + assert_eq!(ctx.value("my_var").unwrap(), Value::from(42)); + assert_eq!(ctx.value("my_func").unwrap(), Value::from(100)); + assert_eq!(ctx.value("non_existent").unwrap(), Value::None); + + // get + assert!(matches!(ctx.get("my_var"), Some(ContextValue::Variable(_)))); + assert!(matches!( + ctx.get("my_func"), + Some(ContextValue::Function(_)) + )); + assert!(matches!(ctx.get("non_existent"), None)); + } +} 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) => {