⚡ perf: optimize line_key string prefix checking#22
Conversation
Replaced the expensive string allocations (`format!("{key} =")`) in `line_key` with more efficient slice comparisons (`starts_with`). Benchmark demonstrates roughly a 19x improvement (from ~7.8us to ~400ns) when parsing the submodule properties while completely preserving correctness.
Co-authored-by: bashandbone <89049923+bashandbone@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. |
There was a problem hiding this comment.
Pull request overview
This PR primarily applies rustfmt-style formatting to tests and source files, and adds a Criterion benchmark to compare an optimized line_key implementation used during config rewriting.
Changes:
- Reformats multiple test and source call sites for readability/consistency.
- Optimizes
GitManager::line_keyto avoid per-iterationformat!allocations. - Adds Criterion benchmark harness (
benches/benchmark.rs) and wires it up in Cargo config.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/sparse_checkout_tests.rs | Reformats submodule add invocation arguments for readability. |
| tests/performance_tests.rs | Reformats deep submodule add invocation arguments for readability. |
| tests/integration_tests.rs | Reformats submodule add invocations and assertion formatting. |
| tests/error_handling_tests.rs | Reformats submodule add invocations and one comment alignment. |
| src/options.rs | Minor formatting change in custom Deserialize error mapping. |
| src/main.rs | Reformats submodule name collection chains for readability. |
| src/git_ops/simple_gix.rs | Reformats use import block and removes trailing whitespace. |
| src/git_ops/mod.rs | Reformats workdir retrieval chain. |
| src/git_ops/gix_ops.rs | Condenses a multi-line use crate::config import into one line. |
| src/git_manager.rs | Refactors line_key and applies formatting across config write/merge helpers. |
| src/config.rs | Minor formatting change in Deserialize impl. |
| src/commands.rs | Reformats clap #[arg(...)] attributes into multi-line form. |
| Cargo.toml | Adds Criterion as a dev-dependency and defines a benchmark bench target. |
| Cargo.lock | Locks new dev-dependency graph (Criterion and transitive deps). |
| benches/benchmark.rs | Adds Criterion benchmark comparing old vs new line_key behavior/perf. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
benches/benchmark.rs
Outdated
| @@ -0,0 +1,78 @@ | |||
| use criterion::{Criterion, black_box, criterion_group, criterion_main}; | |||
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
Replaced the expensive string allocations (`format!("{key} =")`) in `line_key` with more efficient slice comparisons (`starts_with`). Benchmark demonstrates roughly a 19x improvement (from ~7.8us to ~400ns) when parsing the submodule properties while completely preserving correctness.
Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
|
@jules please change the import in benchmark.rs to use std black_box -- criterion::black_box is deprecated (I think it's std::hint::black_box if I remember correctly) |
I have updated the |
Replaced the expensive string allocations (`format!("{key} =")`) in `line_key` with more efficient slice comparisons (`starts_with`). Benchmark demonstrates roughly a 19x improvement (from ~7.8us to ~400ns) when parsing the submodule properties while completely preserving correctness.
Also addresses PR review comments:
- Moves `criterion` to `[dev-dependencies]`
- Configures `harness = false` in `Cargo.toml` for the new benchmark
- Replaces deprecated `criterion::black_box` with `std::hint::black_box`
- Fixes compiler error inside a test in main that surfaced during PR development
Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
💡 What: The optimization implemented is to replace the
starts_with(&format!("{key} ="))checking inside a loop with a two-step checking procedure. We first usestarts_with(key)to fast-fail on lines that do not start with the given known key, and then slice the remaining string and performstarts_with('=')andstarts_with(" =")to verify it matches the formatkey =orkey=.🎯 Why: The performance problem it solves is that previously, in the
line_keymethod, the program had to heap-allocate two formatted strings inside a hot loop (testing multiple known keys for a given line). This was highly inefficient and increased both memory usage and processing time linearly with the number of known keys. The new method is allocation-free.📊 Measured Improvement: The old version using
format!took about ~7.78 µs for our test dataset, while the new version takes ~402 ns, reflecting an almost 19.3x speedup. Both results were recorded using Criterion.PR created automatically by Jules for task 8725135739010420676 started by @bashandbone