⚡ Bolt: optimize Context map lookups and lock contention

Co-authored-by: ashyanSpada <22587148+ashyanSpada@users.noreply.github.com>

Author: google-labs-jules[bot]

.jules/bolt.md

@@ -0,0 +1,4 @@
+
+## 2024-05-28 - [Reduced Context Lock Contention and Double Lookups]
+**Learning:** Holding a lock on global/shared state while executing custom closures in an expression engine can lead to severe lock contention. Furthermore, `HashMap` lookups should always leverage `Option::cloned()` where applicable to avoid redundant matching and cloning.
+**Action:** Always extract the owned value and drop the lock prior to performing long-running or unknown execution times in functions like `Context::value`. Use `.cloned()` and pattern matching directly.

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) {

src/context.rs

@@ -33,34 +33,28 @@ 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();
-        match value {
-            ContextValue::Variable(v) => Ok(v.clone()),
-            ContextValue::Function(func) => func(Vec::new()),
+        match self.get(name) {
+            Some(ContextValue::Variable(v)) => Ok(v),
+            Some(ContextValue::Function(func)) => func(Vec::new()),
+            None => Ok(Value::None),
         }
     }
 }

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) => {