Remove wasted clones of loop-invariant values - #62763
Merged
Merged
Conversation
miguelraz
marked this pull request as draft
August 17, 2026 18:11
zed-community-bot
Bot
added
the
staff
|
@cla-bot check |
cla-bot
Bot
added
the
cla-signed
|
The cla-bot has been summoned, and re-checked this pull request! |
miguelraz
marked this pull request as ready for review
August 27, 2026 05:00
Found by a new deep_clone_in_loop dylint modeled on the accidentally quadratic search fix (#62658). None of these were quadratic in practice, but each paid an avoidable copy: - collab: take() the S3 pagination marker instead of cloning it; it is overwritten or the loop breaks before the next read. - project: build Arc<RelPath> directly from the borrowed path in path_trie, avoiding a clone-then-copy double allocation. - context_server: move the notification payload into the last handler instead of cloning it for every handler. - editor: mem::replace the rewrap accumulators instead of cloning them on every range boundary.
miguelraz
force-pushed
the
remove-wasted-clones-in-loops
branch
from
August 27, 2026 05:00
f893dd7 to
db39a69
Compare
Anthony-Eid
approved these changes
Aug 27, 2026
playdohface
pushed a commit
to playdohface/zed
that referenced
this pull request
Aug 29, 2026
# Objective Remove avoidable clones of loop-invariant values that pay a copy on every loop iteration. These were found by running a prototype `deep_clone_in_loop` dylint (modeled on the accidentally quadratic project-search fix in zed-industries#62658) across the workspace. None of these sites are quadratic in practice — the loops are bounded — but each one does strictly wasted work that a one-line change removes. ## Solution - `collab/src/api/extensions.rs`: use `next_marker.take()` instead of `next_marker.clone()` for the S3 pagination marker. The marker is either overwritten via `clone_from` at the end of the iteration or the loop breaks, so it is never read after the `take`. - `project/src/manifest_tree/path_trie.rs`: build the trie key with `Arc::from(path_so_far.as_rel_path())` instead of `path_so_far.clone().into()`. The `From<RelPathBuf> for Arc<RelPath>` impl copies the bytes into the `Arc` anyway, so the intermediate `RelPathBuf` clone was a second, redundant copy. - `context_server/src/client.rs`: in `notify`, split off the last handler and move the `serde_json::Value` payload into it, cloning only for the preceding handlers. Previously every handler received a clone and the final one was always wasted. - `editor/src/rewrap.rs`: use `std::mem::replace` to move the comment-delimiter and rewrap-prefix accumulators into the pushed range instead of cloning them and immediately reassigning. ## Testing - `cargo check -p collab -p project -p context_server -p editor` — clean. - `cargo test -p editor rewrap` — 7/7 pass, including the boundary-sensitive `test_rewrap*` editor tests. - `cargo test -p context_server` — 97/97 pass. - `cargo test -p project path_trie` — 2/2 pass. - Re-ran the `deep_clone_in_loop` lint on the four crates: the `extensions.rs`, `path_trie.rs`, and `rewrap.rs` hits are gone; `client.rs` still (correctly) reports the remaining required per-handler clone. - Not tested platform-specifically; the changes are platform-independent. ## Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content adheres to Zed's UI standards ([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) and [icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md) guidelines) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable --- Release Notes: - N/A
m-altaifi
pushed a commit
to m-altaifi/zed
that referenced
this pull request
Sep 2, 2026
# Objective Remove avoidable clones of loop-invariant values that pay a copy on every loop iteration. These were found by running a prototype `deep_clone_in_loop` dylint (modeled on the accidentally quadratic project-search fix in zed-industries#62658) across the workspace. None of these sites are quadratic in practice — the loops are bounded — but each one does strictly wasted work that a one-line change removes. ## Solution - `collab/src/api/extensions.rs`: use `next_marker.take()` instead of `next_marker.clone()` for the S3 pagination marker. The marker is either overwritten via `clone_from` at the end of the iteration or the loop breaks, so it is never read after the `take`. - `project/src/manifest_tree/path_trie.rs`: build the trie key with `Arc::from(path_so_far.as_rel_path())` instead of `path_so_far.clone().into()`. The `From<RelPathBuf> for Arc<RelPath>` impl copies the bytes into the `Arc` anyway, so the intermediate `RelPathBuf` clone was a second, redundant copy. - `context_server/src/client.rs`: in `notify`, split off the last handler and move the `serde_json::Value` payload into it, cloning only for the preceding handlers. Previously every handler received a clone and the final one was always wasted. - `editor/src/rewrap.rs`: use `std::mem::replace` to move the comment-delimiter and rewrap-prefix accumulators into the pushed range instead of cloning them and immediately reassigning. ## Testing - `cargo check -p collab -p project -p context_server -p editor` — clean. - `cargo test -p editor rewrap` — 7/7 pass, including the boundary-sensitive `test_rewrap*` editor tests. - `cargo test -p context_server` — 97/97 pass. - `cargo test -p project path_trie` — 2/2 pass. - Re-ran the `deep_clone_in_loop` lint on the four crates: the `extensions.rs`, `path_trie.rs`, and `rewrap.rs` hits are gone; `client.rs` still (correctly) reports the remaining required per-handler clone. - Not tested platform-specifically; the changes are platform-independent. ## Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content adheres to Zed's UI standards ([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) and [icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md) guidelines) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable --- Release Notes: - N/A
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
2 participants
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.
Objective
Remove avoidable clones of loop-invariant values that pay a copy on every loop iteration. These were found by running a prototype
deep_clone_in_loopdylint (modeled on the accidentally quadratic project-search fix in #62658) across the workspace. None of these sites are quadratic in practice — the loops are bounded — but each one does strictly wasted work that a one-line change removes.Solution
collab/src/api/extensions.rs: usenext_marker.take()instead ofnext_marker.clone()for the S3 pagination marker. The marker is either overwritten viaclone_fromat the end of the iteration or the loop breaks, so it is never read after thetake.project/src/manifest_tree/path_trie.rs: build the trie key withArc::from(path_so_far.as_rel_path())instead ofpath_so_far.clone().into(). TheFrom<RelPathBuf> for Arc<RelPath>impl copies the bytes into theArcanyway, so the intermediateRelPathBufclone was a second, redundant copy.context_server/src/client.rs: innotify, split off the last handler and move theserde_json::Valuepayload into it, cloning only for the preceding handlers. Previously every handler received a clone and the final one was always wasted.editor/src/rewrap.rs: usestd::mem::replaceto move the comment-delimiter and rewrap-prefix accumulators into the pushed range instead of cloning them and immediately reassigning.Testing
cargo check -p collab -p project -p context_server -p editor— clean.cargo test -p editor rewrap— 7/7 pass, including the boundary-sensitivetest_rewrap*editor tests.cargo test -p context_server— 97/97 pass.cargo test -p project path_trie— 2/2 pass.deep_clone_in_looplint on the four crates: theextensions.rs,path_trie.rs, andrewrap.rshits are gone;client.rsstill (correctly) reports the remaining required per-handler clone.Self-Review Checklist:
Release Notes: