Skip to content

feat: reduce allocations#1156

Merged
reubeno merged 31 commits into
reubeno:mainfrom
Elsie19:owned_inputs
May 24, 2026
Merged

feat: reduce allocations#1156
reubeno merged 31 commits into
reubeno:mainfrom
Elsie19:owned_inputs

Conversation

@Elsie19

@Elsie19 Elsie19 commented May 13, 2026

Copy link
Copy Markdown
Contributor

Some of the things I did:

  • Initialize with with_capacity where applicable
  • Delay or remove vector initialization
  • Replace individual collection pushes to one extend
  • Replace some function arguments with IntoIterator bounds for looser constraints
  • Use Cow when appropriate
  • Replace Vec in function arguments with &[]
  • Replace map_or_else with map_or when default value is static

I also replaced these functions entirely:

- uppercase_first_char
- lowercase_first_char
+ pattern_to_first_char

and

- uppercase_pattern
- lowercase_pattern
+ pattern_to_string

Where the new functions take a closure over their respective argument and can transform it at the call site. You may want to pull out that specific commit and merge it outside of this PR, but I added it here anyways.

Feel free to pick and choose which ones to keep and leave! If you need I can merge likewise commits into one to make the merge cleaner.

@github-actions

github-actions Bot commented May 13, 2026

Copy link
Copy Markdown

Public API changes for crate: brush-core

Changed items

-pub const fn brush_core::commands::SimpleCommand<'a, SE>::new(shell: brush_core::commands::ShellForCommand<'a, SE>, params: brush_core::ExecutionParameters, command_name: alloc::string::String, args: alloc::vec::Vec<brush_core::commands::CommandArg>) -> Self
+pub fn brush_core::commands::SimpleCommand<'a, SE>::new<I>(shell: brush_core::commands::ShellForCommand<'a, SE>, params: brush_core::ExecutionParameters, command_name: alloc::string::String, args: I) -> Self where I: core::iter::traits::collect::IntoIterator<Item = brush_core::commands::CommandArg>
-pub async fn brush_core::Shell<SE>::invoke_function<N: core::convert::AsRef<str>, I: core::iter::traits::collect::IntoIterator<Item = A>, A: core::convert::AsRef<str>>(&mut self, name: N, args: I, params: &brush_core::ExecutionParameters) -> core::result::Result<u8, brush_core::error::Error>
+pub async fn brush_core::Shell<SE>::invoke_function<N: core::convert::AsRef<str>, I: core::iter::traits::collect::IntoIterator<Item = A>, A: core::convert::AsRef<str>>(&mut self, name: N, args: I, params: brush_core::ExecutionParameters) -> core::result::Result<u8, brush_core::error::Error>

Performance Benchmark Report

Benchmark name Baseline (μs) Test/PR (μs) Delta (μs) Delta %
clone_shell_object 13.54 μs 13.68 μs 0.15 μs 🟠 +1.09%
eval_arithmetic 0.12 μs 0.12 μs 0.00 μs ⚪ Unchanged
expand_one_string 1.22 μs 1.20 μs -0.02 μs ⚪ Unchanged
for_loop 24.57 μs 23.95 μs -0.62 μs 🟢 -2.52%
full_peg_complex 43.41 μs 43.16 μs -0.25 μs ⚪ Unchanged
full_peg_for_loop 4.83 μs 4.79 μs -0.04 μs 🟢 -0.91%
full_peg_nested_expansions 12.92 μs 12.70 μs -0.22 μs ⚪ Unchanged
full_peg_pipeline 3.23 μs 3.24 μs 0.00 μs ⚪ Unchanged
full_peg_simple 1.32 μs 1.35 μs 0.03 μs 🟠 +2.05%
function_call 2.63 μs 2.68 μs 0.05 μs ⚪ Unchanged
instantiate_shell 43.27 μs 43.09 μs -0.18 μs ⚪ Unchanged
instantiate_shell_with_init_scripts 21119.40 μs 21290.80 μs 171.40 μs ⚪ Unchanged
parse_peg_bash_completion 1614.43 μs 1619.21 μs 4.78 μs ⚪ Unchanged
parse_peg_complex 14.70 μs 14.71 μs 0.00 μs ⚪ Unchanged
parse_peg_for_loop 1.51 μs 1.45 μs -0.06 μs 🟢 -4.04%
parse_peg_pipeline 1.56 μs 1.47 μs -0.09 μs 🟢 -5.52%
parse_peg_simple 0.80 μs 0.78 μs -0.02 μs 🟢 -2.62%
run_echo_builtin_command 13.27 μs 13.36 μs 0.09 μs ⚪ Unchanged
tokenize_sample_script 2.85 μs 2.75 μs -0.10 μs 🟢 -3.48%

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
brush-builtins\src\bind.rs 🔴 25.26% 🔴 25.17% 🔴 -0.09%
brush-builtins\src\history.rs 🔴 7.81% 🔴 7.87% 🟢 0.06%
brush-builtins\src\read.rs 🔴 38.24% 🔴 37.93% 🔴 -0.31%
brush-core\src\completion.rs 🔴 14.54% 🔴 14.58% 🟢 0.04%
brush-core\src\expansion.rs 🔴 28.89% 🔴 29.4% 🟢 0.51%
brush-core\src\variables.rs 🔴 10.31% 🔴 10.34% 🟢 0.03%
brush-shell\src\config.rs 🟢 87.85% 🟢 88.26% 🟢 0.41%
Overall Coverage 🟢 25.8% 🟢 25.84% 🟢 0.04%

Minimum allowed coverage is 20%, this run produced 25.84%

Test Summary: bash-completion test suite

Outcome Count Percentage
✅ Pass 1581 74.96
❗️ Error 19 0.90
❌ Fail 155 7.35
⏩ Skip 339 16.07
❎ Expected Fail 13 0.62
✔️ Unexpected Pass 2 0.09
📊 Total 2109 100.00

@Elsie19 Elsie19 marked this pull request as draft May 14, 2026 13:33
@Elsie19 Elsie19 force-pushed the owned_inputs branch 2 times, most recently from 72ad32c to 980cb74 Compare May 14, 2026 13:58
@Elsie19 Elsie19 marked this pull request as ready for review May 14, 2026 14:37
@Elsie19 Elsie19 marked this pull request as draft May 14, 2026 15:12
@Elsie19 Elsie19 marked this pull request as ready for review May 14, 2026 15:42

@lu-zero lu-zero left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I read it twice and it looks very nice.

@Elsie19 Elsie19 marked this pull request as draft May 16, 2026 18:02
@Elsie19 Elsie19 marked this pull request as ready for review May 17, 2026 02:04
{
if let Some(pattern) = pattern {
if !pattern.is_empty() {
let regex = pattern.to_regex(false, false)?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably we could cache them in the future

@reubeno

reubeno commented May 18, 2026

Copy link
Copy Markdown
Owner

Thanks for putting this all togther, @Elsie19!

I've read through everything other than expansion.rs so far -- and it's all looking pretty good. Your changes have left the code cleaner, more idiomatic, and definitely avoided some unneeded allocations 😄.

There are 2 API surface changes flagged by the PR -- I think both of them are warranted, and they're relatively low impact, so I'm supportive of making them.

I'll spend some more time looking through expansion.rs, particularly given that the expand_one_string microbenchmark noted a possible perf change.

@lu-zero

lu-zero commented May 24, 2026

Copy link
Copy Markdown
Contributor

It does crash with #1069 so once it lands I'd re-roll that branch and if we have time would be nice to land it as follow up.

@reubeno

reubeno commented May 24, 2026

Copy link
Copy Markdown
Owner

It does crash with #1069 so once it lands I'd re-roll that branch and if we have time would be nice to land it as follow up.

@lu-zero Any more info on what the crash is? Is there anything you'd recommend get adjusted in this PR before we merge? I just finished reviewing the latest changes (for this PR) and it's looking pretty good to me.

@lu-zero

lu-zero commented May 24, 2026

Copy link
Copy Markdown
Contributor

Just I managed to typoing clash with crash... It works but on the portage-repo it seems to not speed up anything :/

@reubeno reubeno merged commit 504c0b9 into reubeno:main May 24, 2026
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants