Skip to content

refactor: remove cached macro usages from parsers - #1267

Open
lu-zero wants to merge 2 commits into
reubeno:mainfrom
lu-zero:remove-cached-crate
Open

lu-zero wants to merge 2 commits into
reubeno:mainfrom
lu-zero:remove-cached-crate

Conversation

@lu-zero

@lu-zero lu-zero commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Summary

Drop the #[cached(...)] attribute invocations in word, tokenizer, arithmetic, prompt, and shell parsing. Where a public function delegated to a private cached wrapper, the wrapper is inlined into the public function. The now-unused cached dependency is removed from brush-parser; brush-core keeps it because brush-core/src/regex.rs still uses cached::LruCache directly (not the macro).

Motivation

This is an initial experiment to establish a baseline for benchmarking. It removes the cached crate macro usage so we can measure the cost/benefit of the caching before deciding how to replace or tune it.

Changes

  • brush-parser/src/word.rs — inline cacheable_parse into parse
  • brush-parser/src/tokenizer.rs — drop the uncached_tokenize_string wrapper; tokenize_str_with_options calls uncached_tokenize_str directly
  • brush-parser/src/arithmetic.rs — inline cacheable_parse into parse
  • brush-core/src/prompt.rs — drop attribute on parse_prompt
  • brush-core/src/shell/parsing.rs — drop attribute on parse_string_impl
  • brush-parser/Cargo.toml + Cargo.lock — remove cached dependency

Testing

  • cargo clippy --package brush-parser --package brush-core passes
  • cargo test --package brush-parser --package brush-core passes

Assisted-by: opencode:deepseek-v4-flash-free

Drop the `#[cached(...)]` attribute invocations in word, tokenizer,
arithmetic, prompt, and shell parsing, inlining the cached wrapper
functions where they existed. Remove the now-unused `cached`
dependency from brush-parser; brush-core keeps it for the direct
`cached::LruCache` usage in regex.rs.

This is an experiment to obtain a baseline for benchmarking before
deciding how to replace or tune the caching.

Assisted-by: opencode:deepseek-v4-flash-free
@github-actions

github-actions Bot commented Aug 15, 2026

Copy link
Copy Markdown

Performance Benchmark Report

Benchmark name Baseline (μs) Test/PR (μs) Delta (μs) Delta %
clone_shell_object 17.56 μs 17.52 μs -0.04 μs ⚪ Unchanged
eval_arithmetic 0.15 μs 0.91 μs 0.77 μs 🟠 +525.34%
expand_one_string 1.69 μs 3.74 μs 2.05 μs 🟠 +121.37%
for_loop 32.31 μs 64.73 μs 32.41 μs 🟠 +100.30%
full_peg_complex 55.73 μs 54.48 μs -1.25 μs 🟢 -2.25%
full_peg_for_loop 6.07 μs 6.18 μs 0.11 μs 🟠 +1.81%
full_peg_nested_expansions 15.90 μs 16.52 μs 0.62 μs 🟠 +3.87%
full_peg_pipeline 4.12 μs 4.03 μs -0.09 μs ⚪ Unchanged
full_peg_simple 1.76 μs 1.75 μs -0.02 μs 🟢 -1.13%
function_call 3.50 μs 4.49 μs 0.99 μs 🟠 +28.14%
instantiate_shell 56.69 μs 57.54 μs 0.85 μs ⚪ Unchanged
instantiate_shell_with_init_scripts 27382.49 μs 27428.59 μs 46.09 μs ⚪ Unchanged
parse_peg_bash_completion 2082.81 μs 4722.40 μs 2639.59 μs 🟠 +126.73%
parse_peg_complex 19.11 μs 19.12 μs 0.01 μs ⚪ Unchanged
parse_peg_for_loop 1.87 μs 1.84 μs -0.03 μs ⚪ Unchanged
parse_peg_pipeline 1.93 μs 1.96 μs 0.03 μs ⚪ Unchanged
parse_peg_simple 1.00 μs 1.03 μs 0.03 μs 🟠 +3.09%
run_echo_builtin_command 15.87 μs 18.51 μs 2.64 μs 🟠 +16.63%
tokenize_sample_script 3.54 μs 3.58 μs 0.04 μs ⚪ Unchanged

Benchmarks added:

  • run_mixed_commands

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
brush-parser/src/arithmetic.rs 🟢 92.63% 🟢 92.39% 🔴 -0.24%
brush-parser/src/tokenizer.rs 🟢 93.56% 🟢 93.52% 🔴 -0.04%
brush-parser/src/word.rs 🟢 93.82% 🟢 93.74% 🔴 -0.08%
Overall Coverage 🟢 75.97% 🟢 75.96% 🔴 -0.01%

Minimum allowed coverage is 70%, this run produced 75.96%
Maximum allowed coverage difference is -5%, this run produced -0.01%

Test Summary: bash-completion test suite

Outcome Count Percentage
✅ Pass 1581 74.96
❗️ Error 18 0.85
❌ Fail 156 7.40
⏩ Skip 339 16.07
❎ Expected Fail 13 0.62
✔️ Unexpected Pass 2 0.09
📊 Total 2109 100.00

Add two benchmarks that exercise a realistic, mixed set of distinct
commands rather than repeating a single identical command:

- `run_mixed_script`: runs a script containing a variety of shell
  constructs (assignment, arithmetic, expansions, loops, conditionals,
  case, functions, command substitution, redirections).
- `run_mixed_commands`: runs 128 distinct commands one at a time, sized
  to exceed the 64-entry parse cache so most commands miss.

These provide a more representative baseline for evaluating the effect
of the cached-crate parsing caches removed in the preceding commit.

Assisted-by: opencode:deepseek-v4-flash-free
@lu-zero

lu-zero commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Benchmark comparison: cached (main) vs uncached (this branch)

I ran the benchmarks locally on both main (with cached crate) and this branch (without). Besides the existing micro-benchmarks, I added two realistic mixed-command benchmarks (commit a1330ef) and compared the medians.

Delta column = change in the uncached result relative to cached. Positive = uncached is slower; negative = uncached is faster.

Existing micro-benchmarks (repeat the same command every iteration — cache's best case)

Benchmark Cached Uncached Delta (uncached vs cached)
eval_arithmetic 150 ns 981 ns +6.5x (slower)
expand_one_string 1.85 µs 3.95 µs +2.1x (slower)
for_loop 31.6 µs 61.2 µs +1.9x (slower)
run_echo_builtin_command 12.4 µs 14.7 µs +19%
function_call 3.79 µs 4.78 µs +26%
clone_shell_object 14.4 µs 15.5 µs +7.2%
instantiate_shell 38.9 µs 38.9 µs ~0%

New mixed-command benchmarks (added in a1330ef)

Benchmark Cached Uncached Delta (uncached vs cached)
run_mixed_script (same mixed script every iteration) 617.7 µs 747.8 µs +21% (slower)
run_mixed_commands (128 distinct commands, exceeds 64-entry cache) 1.439 ms 1.388 ms −3.5% (faster)

Interpretation

  • When the same input is re-parsed (interactive repeat, tight loops), the cache pays off significantly (~2x-6x in the micro-benchmarks, ~20% for a repeated script).
  • For a pool of distinct commands larger than the 64-entry LruCache, the cache mostly misses and its overhead (key String clones + hashing) makes the cached version slower (~3.5%). This is the realistic case for running varied real-world scripts.

Net takeaway: the cache is a win for repeated-identical-input workloads but roughly a wash-to-slight-loss for varied scripts. This gives us a baseline to decide whether to keep, replace (e.g. a simpler hashmap, larger capacity), or drop the caches.

Note: brush-core still depends on cached for the direct LruCache usage in regex.rs; only the macro-based caches were removed here.

Assisted-by: opencode:deepseek-v4-flash-free

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.

1 participant