⚡ Bolt: Optimize Context lookup and reduce Mutex lock contention

.jules/bolt.md

@@ -0,0 +1,3 @@
+## 2024-03-12 - [Context Map Lookups]
+**Learning:** Found an edge case where map access and function execution was holding a Mutex lock which leads to high lock contention, especially since Context instances store closures that might be arbitrarily expensive. Double map lookups in `.value()` (`is_none()` then `unwrap()`) combined with lock-holding was a bottleneck for expression execution.
+**Action:** Always fetch the target value into an owned variable outside the lock block (using scope bounds or `cloned()`) in high-frequency map lookups. Never execute user closures inside a lock guard.

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

@@ -31,36 +31,32 @@ 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()),
+        match self.get(name)? {
+            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()),
+        match self.get(name)? {
+            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();
+        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),
         }
     }
 }

src/descriptor.rs

@@ -57,9 +57,7 @@ impl DescriptorManager {
     fn get(&self, key: DescriptorKey) -> Option<Descriptor> {
         let binding = self.store.lock().unwrap();
         let value = binding.get(&key);
-        if value.is_none() {
-            return None;
-        }
+        value?;
         Some(value.unwrap().clone())
     }
 

src/function.rs

@@ -16,7 +16,7 @@ impl InnerFunctionManager {
     pub fn new() -> Self {
         static STORE: OnceCell<Mutex<HashMap<String, Arc<InnerFunction>>>> = OnceCell::new();
         let store = STORE.get_or_init(|| Mutex::new(HashMap::new()));
-        InnerFunctionManager { store: store }
+        InnerFunctionManager { store }
     }
 
     pub fn init(&mut self) {

src/lib.rs

@@ -54,7 +54,7 @@ pub fn execute(expr: &str, mut ctx: context::Context) -> Result<Value> {
 /// let ast = parse_expression(input);
 /// assert!(ast.is_ok());
 /// ```
-pub fn parse_expression(expr: &str) -> Result<ExprAST> {
+pub fn parse_expression(expr: &str) -> Result<ExprAST<'_>> {
     init();
     parser::Parser::new(expr)?.parse_stmt()
 }

src/operator.rs

@@ -49,15 +49,15 @@ impl InfixOpManager {
     pub fn new() -> Self {
         static STORE: OnceCell<Mutex<HashMap<String, InfixOpConfig>>> = OnceCell::new();
         let store = STORE.get_or_init(|| Mutex::new(HashMap::new()));
-        InfixOpManager { store: store }
+        InfixOpManager { store }
     }
 
     pub fn init(&mut self) {
         use InfixOpAssociativity::*;
         use InfixOpType::*;
         self.register("=", 20, SETTER, RIGHT, Arc::new(|_, right| Ok(right)));
 
-        for op in vec!["+=", "-=", "*=", "/=", "%="] {
+        for op in ["+=", "-=", "*=", "/=", "%="] {
             self.register(
                 op,
                 20,
@@ -88,7 +88,7 @@ impl InfixOpManager {
             );
         }
 
-        for op in vec!["<<=", ">>=", "&=", "^=", "|="] {
+        for op in ["<<=", ">>=", "&=", "^=", "|="] {
             self.register(
                 op,
                 20,
@@ -109,7 +109,7 @@ impl InfixOpManager {
             );
         }
 
-        for (op, precedence) in vec![("||", 40), ("&&", 50)] {
+        for (op, precedence) in [("||", 40), ("&&", 50)] {
             self.register(
                 op,
                 precedence,
@@ -127,7 +127,7 @@ impl InfixOpManager {
             );
         }
 
-        for op in vec!["<", "<=", ">", ">="] {
+        for op in ["<", "<=", ">", ">="] {
             self.register(
                 op,
                 60,
@@ -148,7 +148,7 @@ impl InfixOpManager {
             );
         }
 
-        for op in vec!["==", "!="] {
+        for op in ["==", "!="] {
             self.register(
                 op,
                 60,
@@ -166,7 +166,7 @@ impl InfixOpManager {
             );
         }
 
-        for (op, precedence) in vec![("|", 70), ("^", 80), ("&", 90), ("<<", 100), (">>", 100)] {
+        for (op, precedence) in [("|", 70), ("^", 80), ("&", 90), ("<<", 100), (">>", 100)] {
             self.register(
                 op,
                 precedence,
@@ -187,7 +187,7 @@ impl InfixOpManager {
             );
         }
 
-        for (op, precedence) in vec![("+", 110), ("-", 110), ("*", 120), ("/", 120), ("%", 120)] {
+        for (op, precedence) in [("+", 110), ("-", 110), ("*", 120), ("/", 120), ("%", 120)] {
             self.register(
                 op,
                 precedence,
@@ -308,7 +308,7 @@ impl InfixOpManager {
         let mut ans = vec![];
         let binding = self.store.lock().unwrap();
         for (op, InfixOpConfig(precedence, _, _, _)) in binding.iter() {
-            ans.push((op.clone(), precedence.clone()));
+            ans.push((op.clone(), *precedence));
         }
         ans.sort_by(|a, b| a.1.cmp(&b.1));
         ans
@@ -324,7 +324,7 @@ impl PrefixOpManager {
     pub fn new() -> Self {
         static STORE: OnceCell<Mutex<HashMap<String, Arc<PrefixOpFunc>>>> = OnceCell::new();
         let store = STORE.get_or_init(|| Mutex::new(HashMap::new()));
-        PrefixOpManager { store: store }
+        PrefixOpManager { store }
     }
 
     pub fn init(&mut self) {
@@ -422,7 +422,7 @@ impl PostfixOpManager {
     pub fn new() -> Self {
         static STORE: OnceCell<Mutex<HashMap<String, Arc<PrefixOpFunc>>>> = OnceCell::new();
         let store = STORE.get_or_init(|| Mutex::new(HashMap::new()));
-        Self { store: store }
+        Self { store }
     }
 
     pub fn init(&mut self) {

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) => {
@@ -154,7 +148,7 @@ impl<'a> ExprAST<'a> {
     }
 
     fn exec_unary(&self, op: &'a str, rhs: &ExprAST, ctx: &mut Context) -> Result<Value> {
-        PrefixOpManager::new().get(&op)?(rhs.exec(ctx)?)
+        PrefixOpManager::new().get(op)?(rhs.exec(ctx)?)
     }
 
     fn exec_binary(
@@ -164,15 +158,15 @@ impl<'a> ExprAST<'a> {
         rhs: &ExprAST<'a>,
         ctx: &mut Context,
     ) -> Result<Value> {
-        match InfixOpManager::new().get_op_type(&op)? {
+        match InfixOpManager::new().get_op_type(op)? {
             InfixOpType::CALC => {
-                InfixOpManager::new().get_handler(&op)?(lhs.exec(ctx)?, rhs.exec(ctx)?)
+                InfixOpManager::new().get_handler(op)?(lhs.exec(ctx)?, rhs.exec(ctx)?)
             }
             InfixOpType::SETTER => {
                 let (a, b) = (lhs.exec(ctx)?, rhs.exec(ctx)?);
                 ctx.set_variable(
                     lhs.get_reference_name()?,
-                    InfixOpManager::new().get_handler(&op)?(a, b)?,
+                    InfixOpManager::new().get_handler(op)?(a, b)?,
                 );
                 Ok(Value::None)
             }
@@ -268,7 +262,7 @@ impl<'a> ExprAST<'a> {
                     "false".into()
                 }
             }
-            String(value) => "\"".to_string() + &value + "\"",
+            String(value) => "\"".to_string() + value + "\"",
         }
     }
 
@@ -298,15 +292,15 @@ impl<'a> ExprAST<'a> {
             let (is, precidence) = lhs.get_precidence();
             let mut tmp: String = lhs.expr();
             if is && precidence < InfixOpManager::new().get_precidence(op) {
-                tmp = "(".to_string() + &lhs.expr() + &")".to_string();
+                tmp = "(".to_string() + &lhs.expr() + ")";
             }
             tmp
         };
         let right = {
             let (is, precidence) = rhs.get_precidence();
             let mut tmp = rhs.expr();
             if is && precidence < InfixOpManager::new().get_precidence(op) {
-                tmp = "(".to_string() + &rhs.expr() + &")".to_string();
+                tmp = "(".to_string() + &rhs.expr() + ")";
             }
             tmp
         };
@@ -326,10 +320,10 @@ impl<'a> ExprAST<'a> {
         for i in 0..params.len() {
             s.push_str(params[i].expr().as_str());
             if i < params.len() - 1 {
-                s.push_str(",");
+                s.push(',');
             }
         }
-        s.push_str("]");
+        s.push(']');
         s
     }
 
@@ -338,13 +332,13 @@ impl<'a> ExprAST<'a> {
         for i in 0..m.len() {
             let (key, value) = m[i].clone();
             s.push_str(key.expr().as_str());
-            s.push_str(":");
+            s.push(':');
             s.push_str(value.expr().as_str());
             if i < m.len() - 1 {
-                s.push_str(",");
+                s.push(',');
             }
         }
-        s.push_str("}");
+        s.push('}');
         s
     }
 
@@ -353,7 +347,7 @@ impl<'a> ExprAST<'a> {
         for i in 0..exprs.len() {
             s.push_str(exprs[i].expr().as_str());
             if i < exprs.len() - 1 {
-                s.push_str(";");
+                s.push(';');
             }
         }
         s
@@ -379,25 +373,25 @@ impl<'a> ExprAST<'a> {
                 op.clone(),
             ),
             Self::List(values) => DescriptorManager::new().get_list_descriptor()(
-                values.into_iter().map(|v| v.describe()).collect(),
+                values.iter().map(|v| v.describe()).collect(),
             ),
             Self::Map(values) => DescriptorManager::new().get_map_descriptor()(
                 values
-                    .into_iter()
+                    .iter()
                     .map(|value| (value.0.describe(), value.1.describe()))
                     .collect(),
             ),
             Self::Function(name, values) => DescriptorManager::new()
                 .get_function_descriptor(name.to_string())(
                 name.to_string(),
-                values.into_iter().map(|v| v.describe()).collect(),
+                values.iter().map(|v| v.describe()).collect(),
             ),
             Self::Reference(name) => DescriptorManager::new()
                 .get_reference_descriptor(name.to_string())(
                 name.to_string()
             ),
             Self::Stmt(values) => DescriptorManager::new().get_chain_descriptor()(
-                values.into_iter().map(|v| v.describe()).collect(),
+                values.iter().map(|v| v.describe()).collect(),
             ),
             Self::Ternary(condition, lhs, rhs) => {
                 DescriptorManager::new().get_ternary_descriptor()(
@@ -416,23 +410,21 @@ pub struct Parser<'a> {
 }
 
 impl<'a> Parser<'a> {
-    fn cur_tok(&self) -> Token {
-        self.tokenizer.cur_token.clone()
+    fn cur_tok(&self) -> Token<'_> {
+        self.tokenizer.cur_token
     }
 
     pub fn new(input: &'a str) -> Result<Self> {
         let mut tokenizer = Tokenizer::new(input);
         tokenizer.next()?;
-        Ok(Self {
-            tokenizer: tokenizer,
-        })
+        Ok(Self { tokenizer })
     }
 
     fn is_eof(&self) -> bool {
         self.cur_tok().is_eof()
     }
 
-    pub fn next(&mut self) -> Result<Token> {
+    pub fn next(&mut self) -> Result<Token<'_>> {
         self.tokenizer.next()
     }
 

src/token.rs

@@ -97,7 +97,7 @@ pub fn check_op(token: Token, expected: &str) -> bool {
         }
         _ => return false,
     }
-    return false;
+    false
 }
 
 impl<'input> Token<'input> {