⚡ Bolt: Optimize context value lookup

.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<ContextValue>`), and release the lock immediately before execution.

src/context.rs

@@ -33,34 +33,30 @@ impl Context {
     pub fn get_func(&self, name: &str) -> Option<Arc<InnerFunction>> {
         let value = self.get(name)?;
         match value {
-            ContextValue::Function(func) => Some(func.clone()),
+            ContextValue::Function(func) => Some(func),
             ContextValue::Variable(_) => None,
         }
     }
 
     pub fn get_variable(&self, name: &str) -> Option<Value> {
         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<ContextValue> {
-        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<Value> {
-        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);
+    }
+}