⚡ Bolt: [performance improvement] optimize Value enum Display allocation#49
⚡ Bolt: [performance improvement] optimize Value enum Display allocation#49ashyanSpada wants to merge 1 commit intomasterfrom
Conversation
Optimized the `std::fmt::Display` trait implementation for the `Value` enum to reduce memory allocations and unnecessary clones. Replaced `format!()` and intermediate `String::from()` buffers with direct writes to the `std::fmt::Formatter` via `write!()`. Removed redundant `.clone()` calls during formatting to avoid unnecessary heap allocations. Trailing delimiters correctly emulate the original format output. Co-authored-by: ashyanSpada <22587148+ashyanSpada@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #49 +/- ##
=======================================
Coverage 89.55% 89.55%
=======================================
Files 11 11
Lines 1063 1063
=======================================
Hits 952 952
Misses 111 111 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request optimizes the Display implementation for the Value enum by removing redundant .clone() calls and replacing intermediate String allocations with direct writes to the formatter. While these changes improve performance, the current implementation for List and Map variants results in trailing separators. It is recommended to use a peekable iterator to ensure a cleaner string representation without trailing commas or semicolons.
| 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, "]") | ||
| } |
There was a problem hiding this comment.
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, "}}") | ||
| } |
There was a problem hiding this comment.
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, "}}")
}There was a problem hiding this comment.
Pull request overview
Optimizes the std::fmt::Display implementation for the Value enum to reduce heap allocations when formatting, especially for collection variants.
Changes:
- Removed unnecessary
.clone()calls when formatting scalarValuevariants. - Reworked
Value::List/Value::Mapformatting to write directly to thefmt::Formatterinstead of building intermediateStrings. - Added a Jules “bolt” note documenting the formatting optimization guideline.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/value.rs |
Streams Display output directly to the formatter to avoid intermediate allocations/clones. |
.jules/bolt.md |
Documents the “avoid intermediate strings/clones in Display” performance learning. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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, "]") | ||
| } | ||
| 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, "}}") |
There was a problem hiding this comment.
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.
💡 What: Optimized the
std::fmt::Displayimplementation for theValueenum by writing directly to thestd::fmt::Formatterstream. This removed intermediateStringcreations (usingformat!()andString::from()) and eliminated redundant.clone()operations.🎯 Why: Generating temporary string buffers and repeatedly cloning inner values adds unnecessary heap allocations and overhead when formatting large collections like
Value::ListandValue::Map. Directly writing to the formatter stream is more idiomatic and performant in Rust.📊 Impact: Reduces peak memory usage and avoids unneeded heap allocations during stringification of expressions. Lowers the overhead of executing display and logging functionalities involving the
Valuetype.🔬 Measurement: Execute
cargo benchand observe the reduction in memory pressure or the slight improvement in average throughput when repeatedly evaluating expressions containing lists and maps. Verify original format functionality usingcargo test.PR created automatically by Jules for task 2694804711574819080 started by @ashyanSpada