Skip to content

⚡ Bolt: [performance improvement] optimize Value enum Display allocation #49

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ashyanSpada wants to merge 1 commit into
base: master
Choose a base branch
from
Open

⚡ Bolt: [performance improvement] optimize Value enum Display allocation #49

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .jules/bolt.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,7 @@
## 2024-05-26 - Optimize decimal conversion in Value
**Learning:** `rust_decimal::Decimal` allows efficient and direct conversion to basic types like `i64` and `f64` via `to_i64` and `to_f64` using the `rust_decimal::prelude::ToPrimitive` trait. Converting to string and then parsing `val.to_string().parse()` is an anti-pattern as it incurs heap allocation overhead, which is bad for performance. When doing integer conversions from `Decimal`, it's critical to check `val.scale() == 0` first to maintain behavioral parity with string parsing (which would reject floats).
**Action:** Always favor direct conversion traits like `rust_decimal::prelude::ToPrimitive` methods over stringification when converting `Decimal` values to native numeric types. Keep edge cases like `scale()` properties in mind when changing conversion methods.

## 2024-05-27 - Avoid intermediate strings and clones in std::fmt::Display
**Learning:** For `std::fmt::Display` implementations involving collections (like Lists or Maps), building an intermediate `String` via `format!()` and `push_str()` before writing it causes unnecessary heap allocations and redundant data cloning.
**Action:** Write directly to the `std::fmt::Formatter` using `write!(f, ...)` to stream characters without allocating extra memory, and remove unnecessary `.clone()` calls during formatting.
21 changes: 9 additions & 12 deletions src/value.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,25 +17,22 @@ pub enum Value {
impl fmt::Display for Value {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Self::String(val) => write!(f, "value string: {}", val.clone()),
Self::Number(val) => write!(f, "value number: {}", val.clone()),
Self::Bool(val) => write!(f, "value bool: {}", val.clone()),
Self::String(val) => write!(f, "value string: {}", val),
Self::Number(val) => write!(f, "value number: {}", val),
Self::Bool(val) => write!(f, "value bool: {}", val),
Self::List(values) => {
let mut s = String::from("[");
write!(f, "value list: [")?;
for value in values {
s.push_str(format!("{},", value.clone()).as_str());
write!(f, "{},", value)?;
}
s.push_str("]");
write!(f, "value list: {}", s)
write!(f, "]")
}
Comment on lines 23 to 29
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Mar 27, 2026

Choose a reason for hiding this comment

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

medium

The current implementation for formatting a List produces a trailing comma (e.g., [v1,v2,]). While this change preserves the original behavior, it's generally better to produce a clean, comma-separated list without a trailing separator. This improves the readability and correctness of the string representation. You can achieve this by using a peekable iterator to check if it's the last element before printing the comma.

            Self::List(values) => {
                write!(f, "value list: [")?;
                let mut iter = values.iter().peekable();
                while let Some(value) = iter.next() {
                    write!(f, "{}", value)?;
                    if iter.peek().is_some() {
                        write!(f, ",")?;
                    }
                }
                write!(f, "]")
            }

Self::Map(m) => {
let mut s = String::from("{");
write!(f, "value map: {{")?;
for (k, v) in m {
s.push_str(format!("key: {},", k.clone()).as_str());
s.push_str(format!("value: {}; ", v.clone()).as_str());
write!(f, "key: {},value: {}; ", k, v)?;
}
s.push_str("}");
write!(f, "value map: {}", s)
write!(f, "}}")
Comment on lines 23 to +35
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

The fmt::Display output for Value has changed implementation significantly (streaming writes vs building a String), but there are no tests asserting the formatted output for each variant (especially List/Map). Since this is user-visible behavior, please add unit tests that compare Value::to_string() against the expected strings for representative values (empty/non-empty list, map with multiple entries, nested values) to prevent accidental format regressions.

Copilot uses AI. Check for mistakes.
}
Comment on lines 30 to 36
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Mar 27, 2026

Choose a reason for hiding this comment

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

medium

Similar to the List implementation, the Map formatting produces a trailing separator and space (; ), for example {key: k,value: v; }. This can be avoided to create a cleaner output string. Using peekable() on the iterator is an idiomatic way to handle this and avoid the trailing separator.

            Self::Map(m) => {
                write!(f, "value map: {{")?;
                let mut iter = m.iter().peekable();
                while let Some((k, v)) = iter.next() {
                    write!(f, "key: {},value: {}", k, v)?;
                    if iter.peek().is_some() {
                        write!(f, "; ")?;
                    }
                }
                write!(f, "}}")
            }

Self::None => write!(f, "None"),
}
Expand Down
Loading