perf: reuse normalized path buffers#10315
Merged
Merged
Conversation
✅ Deploy Preview for rolldown-rs canceled.
|
hyfdev
marked this pull request as ready for review
July 16, 2026 13:12
hyfdev
requested review from
IWANABETHATGUY,
h-a-n-a,
sapphi-red and
shulaoda
as code owners
July 16, 2026 13:12
Member
Author
Merge activity
|
hyfdev
marked this pull request as draft
July 16, 2026 13:17
hyfdev
marked this pull request as ready for review
July 16, 2026 13:20
sapphi-red
approved these changes
Jul 17, 2026
## Summary - consume joined `PathBuf` values through a shared `normalize_path_buf_to_slash` helper - avoid copying the normalized glob path into a second allocation before matching - strengthen the `./foo` versus `foo` regression test through the public filter path, including a Windows drive cwd - document the owned-path rule in the path manipulation guide and link it from `AGENTS.md` ## Why #10313 correctly inserted lexical normalization into the existing `join -> to_string_lossy -> slash` chain. However, `join` had already produced an owned `PathBuf`; calling the non-consuming `normalize()` API converted it to `Cow<Path>`, and the following string conversion copied the normalized buffer. The same ownership issue was caught during #10230's review in `file_emitter.rs`, but the general rule never made it into the final style guide. The guide covered consuming relative/slash conversion and exposed `path_buf_to_slash`, but it did not provide a named normalize-plus-slash helper or list the non-consuming owned-path chain as an anti-pattern. This PR records that missing rule and centralizes the composition so future call sites do not need to reconstruct it. ## Performance A release-mode `GlobalAlloc` counter around matcher construction reported one fewer allocation/reallocation for each non-bypass relative glob: | Pattern | Current | This PR | | --- | ---: | ---: | | `foo/*.txt` | 3 | 2 | | `./foo/*.txt` | 4 | 3 | The probe compared exact outputs across clean, dirty, parent, trailing-separator, absolute, and `**` cases. No benchmark case is added here; if this path is added to continuous performance coverage, the benchmark should land first in a baseline-only PR and the implementation comparison should follow from that baseline. ## Validation - `just lint-rust` - `just lint-repo` - `cargo test -p rolldown_std_utils -p rolldown_utils -p rolldown_common` - `cargo check -p rolldown_std_utils -p rolldown_utils --tests --target x86_64-pc-windows-msvc --locked` - `cargo check -p rolldown_std_utils --tests --target wasm32-wasip1 --locked` - `RUSTDOCFLAGS='-D warnings' cargo doc -p rolldown_std_utils --no-deps --locked` - `just test-rust` executed the workspace suite successfully until the single `test262_module_code` test stopped on the intentionally uninitialized `test262` submodule; 1,873 integration tests had passed, with no code failure - adversarial review passed after documenting the strict UTF-8 panic contract
graphite-app
Bot
force-pushed
the
agent/reuse-glob-path-buffer
branch
from
July 17, 2026 09:42
49fcbb0 to
0e1e8aa
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PathBufvalues through a sharednormalize_path_buf_to_slashhelper./fooversusfooregression test through the public filter path, including a Windows drive cwdAGENTS.mdWhy
#10313 correctly inserted lexical normalization into the existing
join -> to_string_lossy -> slashchain. However,joinhad already produced an ownedPathBuf; calling the non-consumingnormalize()API converted it toCow<Path>, and the following string conversion copied the normalized buffer.The same ownership issue was caught during #10230's review in
file_emitter.rs, but the general rule never made it into the final style guide. The guide covered consuming relative/slash conversion and exposedpath_buf_to_slash, but it did not provide a named normalize-plus-slash helper or list the non-consuming owned-path chain as an anti-pattern.This PR records that missing rule and centralizes the composition so future call sites do not need to reconstruct it.
Performance
A release-mode
GlobalAlloccounter around matcher construction reported one fewer allocation/reallocation for each non-bypass relative glob:foo/*.txt./foo/*.txtThe probe compared exact outputs across clean, dirty, parent, trailing-separator, absolute, and
**cases. No benchmark case is added here; if this path is added to continuous performance coverage, the benchmark should land first in a baseline-only PR and the implementation comparison should follow from that baseline.Validation
just lint-rustjust lint-repocargo test -p rolldown_std_utils -p rolldown_utils -p rolldown_commoncargo check -p rolldown_std_utils -p rolldown_utils --tests --target x86_64-pc-windows-msvc --lockedcargo check -p rolldown_std_utils --tests --target wasm32-wasip1 --lockedRUSTDOCFLAGS='-D warnings' cargo doc -p rolldown_std_utils --no-deps --lockedjust test-rustexecuted the workspace suite successfully until the singletest262_module_codetest stopped on the intentionally uninitializedtest262submodule; 1,873 integration tests had passed, with no code failure