Skip to content

⚡ Bolt: Optimize fmt::Display for Value to reduce memory allocations #51

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: Optimize fmt::Display for Value to reduce memory allocations #51

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 - Optimize fmt::Display for Value to eliminate intermediate string allocations
**Learning:** For `std::fmt::Display` implementations involving collections (like Lists or Maps), writing directly to the `std::fmt::Formatter` using `write!(f, ...)` avoids unnecessary heap allocations and redundant cloning caused by intermediate `String` creation (e.g., via `format!()` and `push_str()`).
**Action:** Always favor direct formatting with `write!(f, ...)` and avoid `clone()` or intermediate `String` allocations when implementing `Display` or generating complex strings.
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)?;
}
Comment on lines 25 to 27
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Mar 30, 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 of Display for Value::List results in a trailing comma (e.g., [1,2,]). While this refactoring correctly optimizes memory allocations, it is also an opportunity to improve the output format by removing the trailing separator for a more standard representation.

Suggested change
for value in values {
s.push_str(format!("{},", value.clone()).as_str());
write!(f, "{},", value)?;
}
for (i, value) in values.iter().enumerate() {
if i > 0 {
write!(f, ",")?;
}
write!(f, "{}", value)?;
}

s.push_str("]");
write!(f, "value list: {}", s)
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)?;
}
Comment on lines 32 to 34
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Mar 30, 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 leaves a trailing semicolon and space (e.g., {key: k,value: v; }). Consider using an enumeration to avoid the trailing separator and improve the readability of the Display output.

                for (i, (k, v)) in m.iter().enumerate() {
                    if i > 0 {
                        write!(f, "; ")?;
                    }
                    write!(f, "key: {},value: {}", k, v)?;
                }

s.push_str("}");
write!(f, "value map: {}", s)
write!(f, "}}")
Comment on lines 17 to +35
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

fmt::Display for Value was refactored to stream directly into the formatter. Since this is user-visible formatting (and is used by benches), it would be good to add unit tests that assert the exact format!("{}", Value::...) output for representative cases (including nested List/Map) to prevent accidental formatting regressions during future perf refactors.

Copilot uses AI. Check for mistakes.
}
Self::None => write!(f, "None"),
}
Expand Down
Loading