diff --git a/.jules/bolt.md b/.jules/bolt.md index 5e43570..9d407c0 100644 --- a/.jules/bolt.md +++ b/.jules/bolt.md @@ -1,3 +1,7 @@ ## 2024-05-24 - Avoid vec! for constant collections in initialization loops **Learning:** Initializing maps/operator managers by iterating over `vec![...]` causes unnecessary heap allocations. Using array literals `[...]` is significantly more efficient since the size is known at compile time and the arrays can be stack-allocated or embedded directly into the binary. **Action:** Always prefer iterating over array literals instead of `vec![...]` for statically known collections, especially in hot paths or initialization loops. + +## 2024-05-25 - Avoid double lookup and lock contention in Context lookup +**Learning:** `HashMap::get(name)` followed by `.unwrap()` inside a Mutex lock not only does double lookup but keeps the lock longer than necessary when executing an inner function or cloning a value. +**Action:** Use a single lookup, clone the value or arc into a local variable (`Option`), and release the lock immediately before execution. diff --git a/src/context.rs b/src/context.rs index 7d987cb..61a5a3b 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,26 +41,22 @@ 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()) + self.0.lock().unwrap().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(); + // Lock is acquired and released inside `get` early to avoid contention during execution + let value = self.get(name); 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), } } } @@ -104,3 +100,30 @@ macro_rules! create_context { ctx }}; } + +#[cfg(test)] +mod tests { + use crate::context::Context; + use crate::value::Value; + use std::sync::Arc; + + #[test] + fn test_context() { + let mut ctx = Context::new(); + + ctx.set_variable("a", Value::from(10)); + ctx.set_func("f", Arc::new(|_| Ok(Value::from(20)))); + + assert_eq!(ctx.get_variable("a").unwrap(), Value::from(10)); + assert!(ctx.get_variable("f").is_none()); + assert!(ctx.get_variable("missing").is_none()); + + assert!(ctx.get_func("f").is_some()); + assert!(ctx.get_func("a").is_none()); + assert!(ctx.get_func("missing").is_none()); + + assert_eq!(ctx.value("a").unwrap(), Value::from(10)); + assert_eq!(ctx.value("f").unwrap(), Value::from(20)); + assert_eq!(ctx.value("missing").unwrap(), Value::None); + } +}