Skip to content

Consolidate Bolt PRs: optimize Context lock usage and eliminate redundant ContextValue clones#59

Merged
ashyanSpada merged 2 commits intomasterfrom
copilot/consolidate-bolt-performance-improvements
Apr 6, 2026
Merged

Consolidate Bolt PRs: optimize Context lock usage and eliminate redundant ContextValue clones#59
ashyanSpada merged 2 commits intomasterfrom
copilot/consolidate-bolt-performance-improvements

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 6, 2026

21 overlapping AI-generated "⚡ Bolt:" PRs (#34#57) targeting the same three areas (Value display, Context locking, token string allocations) were clogging the PR queue. This consolidates the remaining valid improvements into a single clean change.

Already on master (prior commit)

  • Value::fmt streams directly to fmt::Formatter via write! — no intermediate String buffers
  • DelimTokenType::as_str() returns &'static str; check_op() and tokenizer::expect() use it directly

This PR: src/context.rs

get_func / get_variable — previously delegated to self.get() which calls .cloned() on the full ContextValue enum regardless of variant. Now acquires the lock directly and matches on a borrowed reference, cloning only the specific inner value when the variant matches:

// Before: clones entire ContextValue (including Value or Arc) before variant check
pub fn get_variable(&self, name: &str) -> Option<Value> {
    let value = self.get(name)?;          // full ContextValue clone
    match value {
        ContextValue::Variable(v) => Some(v),
        ContextValue::Function(_) => None, // Arc was cloned and thrown away
    }
}

// After: borrows from the locked map, clones only if variant matches
pub fn get_variable(&self, name: &str) -> Option<Value> {
    let binding = self.0.lock().unwrap();
    match binding.get(name)? {
        ContextValue::Variable(v) => Some(v.clone()),
        ContextValue::Function(_) => None,
    }
}

value() — explicit lock scope block ensures MutexGuard is dropped before invoking any stored function, closing a potential deadlock window in reentrant contexts.

Original prompt

Triage and Close Redundant AI-Generated "Bolt" Pull Requests

The repository currently has a large backlog of AI-generated pull requests (all prefixed with "⚡ Bolt:") that are clogging the PR queue and making it difficult to find genuine community contributions. These PRs span from #34 to #57 and all target overlapping concerns: Value display allocations, Context lock contention, and token string allocations.

Task

Review all open "⚡ Bolt:" PRs and implement the best, non-overlapping performance improvements directly on the master branch in a single, clean PR. This consolidates the valid work while eliminating the noise.

The Bolt PRs to consolidate are:

  • #57 — eliminate token string allocations
  • #56 — Zero-allocation string evaluation for Token checking
  • #55 — Optimize Display Formatting via Direct Stream Writes
  • #54 — Optimize Value Display Allocations
  • #53 — Avoid String heap allocation in DelimTokenType
  • #52 — Optimize Display implementation for Value by removing heap allocations
  • #51 — Optimize fmt::Display for Value to reduce memory allocations
  • #50 — Optimize Value Display Formatting
  • #49 — optimize Value enum Display allocation
  • #48 — Optimize fmt::Display for Value collections
  • #45 — Optimize Value enum Display to avoid string allocations
  • #43 — Optimize Context state access and lock contention
  • #42 — optimize Context map lookups and lock contention
  • #41 — optimize Context map lookups and lock contention
  • #40 — Optimize Context lookups and lock contention
  • #39 — Optimize Context lookups to avoid double-lookups and unnecessary .clone()s
  • #38 — Optimize Value Display formatting
  • #37 — Optimize Context variable and function lookups
  • #36 — Optimize Context value retrieval and lock duration
  • #35 — Reduce Mutex lock contention in Context
  • #34 — Optimize Context lookup and reduce Mutex lock contention

What to implement

Read the source files (src/value.rs, src/context.rs, src/token.rs, src/tokenizer.rs) and apply the best, correctness-preserving performance improvements from the Bolt PRs directly. Specifically:

  1. src/value.rs — Optimize fmt::Display for Value to write directly to the formatter instead of building intermediate String allocations (e.g., avoid format!(...).fmt(f) patterns).

  2. src/context.rs — Eliminate double-lookups in the Context maps (e.g., use entry() API or single get/insert calls instead of contains_key + insert), and minimize Mutex lock duration by dropping the guard early where possible.

  3. src/token.rs / src/tokenizer.rs — Where token type comparisons build temporary Strings just to compare, replace with &str slices or direct char/pattern comparisons to avoid heap allocations.

Constraints

  • All existing tests must continue to pass.
  • Do not change the public API surface.
  • Keep the changes minimal, correct, and idiomatic Rust.
  • This is a consolidation — do NOT introduce speculative or unproven optimizations; only apply clear, safe improvements visible in the existing code.

The following is the prior conversation context from the user's chat exploration (may be truncated):

User: Review this repo

Assistant: Here's a comprehensive review of ashyanSpada/expression_engine_rs — a pure Rust expression/rule engine library.


📊 At a Glance

Metric Value
Language Rust (100%)
Version 0.7.0
License Apache 2.0
Stars ⭐ 34
Forks 4
Created January 2023
Last Push April 5, 2026
Open Issues/PRs 26

✅ Strengths

1. Clean, Well-Structured Codebase

The `...

This pull request was created from Copilot chat.

…et_func/get_variable/value

Agent-Logs-Url: https://github.com/ashyanSpada/expression_engine_rs/sessions/7e9ce6a0-1aa8-4510-9420-d73ff44eee82

Co-authored-by: ashyanSpada <22587148+ashyanSpada@users.noreply.github.com>
Copilot AI changed the title [WIP] Consolidate and optimize non-overlapping bolt PRs Consolidate Bolt PRs: optimize Context lock usage and eliminate redundant ContextValue clones Apr 6, 2026
Copilot AI requested a review from ashyanSpada April 6, 2026 16:11
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.35%. Comparing base (f50ac5d) to head (3bc70e3).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
- Coverage   89.53%   89.35%   -0.18%     
==========================================
  Files          11       11              
  Lines        1061     1062       +1     
==========================================
- Hits          950      949       -1     
- Misses        111      113       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ashyanSpada ashyanSpada marked this pull request as ready for review April 6, 2026 16:28
Copilot AI review requested due to automatic review settings April 6, 2026 16:28
@ashyanSpada ashyanSpada merged commit feea6b7 into master Apr 6, 2026
14 of 15 checks passed
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Consolidates remaining Bolt optimizations by improving Context read-path performance and correctness around mutex lock scope during function evaluation.

Changes:

  • Updated get_func / get_variable to avoid cloning the full ContextValue enum when only one variant is needed.
  • Refactored Context::value() to ensure the mutex guard is dropped before invoking stored functions (reducing contention and avoiding re-entrant deadlocks).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +54 to +58
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(),
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Context::value now intentionally drops the MutexGuard before invoking a stored function (to avoid re-entrant deadlocks). Since this is a behavioral/concurrency fix, please add a regression test that would have deadlocked previously: store a function that calls back into the same Context (e.g., ctx.value("a")), call ctx.value("f") on another thread, and assert it completes via recv_timeout within a short duration.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants