Skip to content

⚡ Bolt: Optimize fmt::Display for Value collections #48

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 collections #48

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 Display implementations for collections
**Learning:** `fmt::Display` implementations involving collections (like Lists or Maps) can be highly inefficient if they build intermediate `String`s via `format!()` and `push_str()` inside loops. This causes unnecessary heap allocations.
**Action:** Always write directly to the `fmt::Formatter` inside a loop using `write!(f, ...)` to minimize heap allocations and improve formatting performance.
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 26, 2026

Choose a reason for hiding this comment

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

medium

While this implementation is more performant, it retains the trailing comma in the output for lists (e.g., [item1,item2,]). This can be undesirable. You can avoid the trailing comma without sacrificing performance by handling the separator explicitly between items. This produces a cleaner, more standard output format.

Suggested change
for value in values {
s.push_str(format!("{},", value.clone()).as_str());
write!(f, "{},", value)?;
}
if let Some((first, rest)) = values.split_first() {
write!(f, "{}", first)?;
for value in rest {
write!(f, ",{}", value)?;
}
}

s.push_str("]");
write!(f, "value list: {}", s)
write!(f, "]")
Comment on lines 23 to +28
Copy link

Copilot AI Mar 26, 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::List is being generated via streaming writes now, but there are no unit tests in this file asserting the exact string representation (including the trailing commas). Add a regression test for list formatting (empty + non-empty + nested values) to ensure future refactors don’t accidentally change the output.

Copilot uses AI. Check for mistakes.
}
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 26, 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 formatting, the map formatting leaves a trailing separator ('; ') at the end of the output. This can be avoided for a cleaner output. You can adapt the logic to only print separators between key-value pairs.

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, "; ")?;
    }
}

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

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

The streamed fmt::Display formatting for Value::Map is performance-improved, but there’s no unit test asserting the exact output string (including delimiters, spacing, and the trailing ; per entry). Add a regression test that formats a small map (including empty) and compares to the expected string to lock in the behavior.

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