perf(ops): panel refresh, search, fs, archive, copy hot paths - #111
Conversation
Reviewer's GuideThis PR streamlines several file/search operations and panel refresh paths for performance, primarily by avoiding redundant work (extra sorts, stats, allocations, memmem table rebuilds) and by introducing cheaper data representations and fast paths in hot code. Sequence diagram for updated content search memmem_Finder reusesequenceDiagram
actor User
participant SearchAPI as search_content
participant Finder as memmem_Finder
participant Ctx as ContentSearchContext
participant Rec as search_content_recursive_inner
participant Proc as process_content_entry
participant FileSearch as search_in_file
participant ScanCtx as ScanContext
participant Scan as scan_lines
participant Record as try_record_line
User->>SearchAPI: search_content(path, pattern, case_sensitive, cancel)
SearchAPI->>Finder: memmem::Finder::new(pattern_bytes)
SearchAPI->>Ctx: ContentSearchContext{pattern, case_sensitive, finder, recursive, outcome}
SearchAPI->>Rec: search_content_recursive_inner(path, depth=0, &mut Ctx)
Rec->>Proc: process_content_entry(entry, path, depth, &mut Ctx)
Proc->>FileSearch: search_in_file(&entry_path, pattern, case_sensitive, Ctx.finder, Ctx.outcome, Ctx.cancel)
FileSearch->>ScanCtx: ScanContext{path, case_sensitive, finder, bufs, cancel}
FileSearch->>Scan: scan_lines(&mut ScanCtx, reader, outcome)
loop per_chunk
Scan->>Record: try_record_line(&mut ScanCtx, outcome, &mut file_path, line_start, line_end, line_no)
alt first_match_in_file
Record->>Record: file_path.get_or_insert_with(|| Arc::from(ctx.path))
end
Record->>Record: line_contains_needle(ScanCtx.finder, line, line_text, ci_buf)
Record-->>Scan: match_recorded
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
CompiledPattern::try_simple_wildcard, collectingstar_positionsinto aVecon every call adds an allocation to a hot path; you could retain the single-pass logic but track count/first/last positions with simple variables instead of allocating. - The
rename_entrysame_filecomputation usingmatch (new_meta.as_ref(), new_meta.is_some())is a bit hard to follow; consider a clearerif let Some(new_meta) = &new_meta { ... }structure to make the control flow and same-inode check more readable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `CompiledPattern::try_simple_wildcard`, collecting `star_positions` into a `Vec` on every call adds an allocation to a hot path; you could retain the single-pass logic but track count/first/last positions with simple variables instead of allocating.
- The `rename_entry` `same_file` computation using `match (new_meta.as_ref(), new_meta.is_some())` is a bit hard to follow; consider a clearer `if let Some(new_meta) = &new_meta { ... }` structure to make the control flow and same-inode check more readable.
## Individual Comments
### Comment 1
<location path="src/ops/search/walk.rs" line_range="61-63" />
<code_context>
-pub(super) fn item_limit_reached<T>(
- outcome: &mut SearchOutcome<T, SearchError>,
+///
+/// Non-generic: the type parameter is unused, so genericizing would produce
+/// duplicate monomorphized copies with no benefit.
+pub(super) fn item_limit_reached<E>(
+ outcome: &mut SearchOutcome<E, SearchError>,
max_items: usize,
</code_context>
<issue_to_address>
**issue:** The comment claims `item_limit_reached` is non-generic, but the function still has an unused generic type parameter.
The helper is still generic over `E` via `SearchOutcome<E, SearchError>`, so the monomorphization cost remains and `E` is unused in the body. The doc comment now contradicts the signature and may trigger an unused type parameter warning.
Please either keep it generic and adjust the comment, or remove the type parameter by using a concrete `SearchOutcome` type if possible, so comment and implementation are aligned.
</issue_to_address>
### Comment 2
<location path="src/ops/file_ops/entry_ops.rs" line_range="98-99" />
<code_context>
- (Ok(old_meta), Some(new_meta)) => super::common::same_inode(&old_meta, new_meta),
+ // Only stat `old` when the dest exists (for the same-inode check).
+ // When dest doesn't exist, the old stat is unnecessary.
+ let same_file = match (new_meta.as_ref(), new_meta.is_some()) {
+ (Some(new_meta), true) => match fs::symlink_metadata(old) {
+ Ok(old_meta) => super::common::same_inode(&old_meta, new_meta),
+ _ => false,
</code_context>
<issue_to_address>
**suggestion:** The `match` on `(new_meta.as_ref(), new_meta.is_some())` is more complex than needed and slightly obscures intent.
The previous version matched directly on `(fs::symlink_metadata(old), new_meta.as_ref())`, which was clearer. The new code adds a redundant boolean arm and nests an extra `match`, making it harder to read.
You can keep the same behavior and “only stat `old` when dest exists” optimization with a simpler structure:
```rust
let same_file = match new_meta.as_ref() {
Some(new_meta) => match fs::symlink_metadata(old) {
Ok(old_meta) => super::common::same_inode(&old_meta, new_meta),
_ => false,
},
None => false,
};
```
This removes the unnecessary boolean and clarifies intent.
Suggested implementation:
```rust
// Only stat `old` when the dest exists (for the same-inode check).
// When dest doesn't exist, the old stat is unnecessary.
let same_file = match new_meta.as_ref() {
Some(new_meta) => match fs::symlink_metadata(old) {
Ok(old_meta) => super::common::same_inode(&old_meta, new_meta),
_ => false,
},
None => false,
};
```
No additional changes are required; this preserves the "only stat `old` when dest exists" behavior while simplifying the control flow and clarifying intent.
</issue_to_address>
### Comment 3
<location path="src/ops/search/content.rs" line_range="143-110" />
<code_context>
+/// Like `search_content_recursive_inner` but skips the `path.is_dir()` check —
+/// the caller (process_content_entry) already confirmed the type from the
+/// dirent, so a second stat syscall is unnecessary.
+fn search_content_recursive_inner_skip_dir_check(
+ path: &Path,
+ depth: usize,
+ ctx: &mut ContentSearchContext<'_>,
+) {
+ if ctx.cancel.load(Ordering::Relaxed) {
return;
}
</code_context>
<issue_to_address>
**suggestion:** The new `_skip_dir_check` variant duplicates most of `search_content_recursive_inner`; consider consolidating to reduce divergence risk.
These two functions now differ only in the initial `is_dir` check; the rest of the traversal logic is duplicated. This increases the risk that future changes to recursive traversal will be applied to one function but not the other. Consider factoring the common logic into a shared helper (e.g., accepting a `skip_dir_check` flag or moving the `depth == 0 && !path.is_dir()` guard to the caller) so that specialized entry points still exist but the traversal is defined in a single place.
Suggested implementation:
```rust
fn search_content_recursive_inner_skip_dir_check(
path: &Path,
depth: usize,
ctx: &mut ContentSearchContext<'_>,
) {
// Delegate to the shared traversal helper, skipping the initial is_dir check.
search_content_recursive_inner_common(path, depth, ctx, true);
}
```
To fully implement the consolidation and avoid duplicated traversal logic, the following changes are also needed elsewhere in `src/ops/search/content.rs`:
1. Introduce a shared helper that contains the traversal logic and takes a flag indicating whether to skip the initial `is_dir` check, e.g.:
```rust
fn search_content_recursive_inner_common(
path: &Path,
depth: usize,
ctx: &mut ContentSearchContext<'_>,
skip_dir_check: bool,
) {
if ctx.cancel.load(Ordering::Relaxed) {
return;
}
if !skip_dir_check && depth == 0 && !path.is_dir() {
return;
}
// Move the existing recursive traversal body from
// `search_content_recursive_inner` into this function.
// ...
}
```
2. Refactor the original `search_content_recursive_inner` to delegate to the helper while preserving its current behavior:
```rust
fn search_content_recursive_inner(
path: &Path,
depth: usize,
ctx: &mut ContentSearchContext<'_>,
) {
search_content_recursive_inner_common(path, depth, ctx, false);
}
```
3. Remove the now-duplicated traversal code from `search_content_recursive_inner` and `search_content_recursive_inner_skip_dir_check`, ensuring that all future changes to the recursive traversal are made only in `search_content_recursive_inner_common`.
You will need to adapt the contents of `search_content_recursive_inner_common` to match the existing traversal logic currently implemented in `search_content_recursive_inner`, including any `process_content_entry` calls and depth handling that are not visible in the provided snippet.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| /// Non-generic: the type parameter is unused, so genericizing would produce | ||
| /// duplicate monomorphized copies with no benefit. | ||
| pub(super) fn item_limit_reached<E>( |
There was a problem hiding this comment.
issue: The comment claims item_limit_reached is non-generic, but the function still has an unused generic type parameter.
The helper is still generic over E via SearchOutcome<E, SearchError>, so the monomorphization cost remains and E is unused in the body. The doc comment now contradicts the signature and may trigger an unused type parameter warning.
Please either keep it generic and adjust the comment, or remove the type parameter by using a concrete SearchOutcome type if possible, so comment and implementation are aligned.
| let same_file = match (new_meta.as_ref(), new_meta.is_some()) { | ||
| (Some(new_meta), true) => match fs::symlink_metadata(old) { |
There was a problem hiding this comment.
suggestion: The match on (new_meta.as_ref(), new_meta.is_some()) is more complex than needed and slightly obscures intent.
The previous version matched directly on (fs::symlink_metadata(old), new_meta.as_ref()), which was clearer. The new code adds a redundant boolean arm and nests an extra match, making it harder to read.
You can keep the same behavior and “only stat old when dest exists” optimization with a simpler structure:
let same_file = match new_meta.as_ref() {
Some(new_meta) => match fs::symlink_metadata(old) {
Ok(old_meta) => super::common::same_inode(&old_meta, new_meta),
_ => false,
},
None => false,
};This removes the unnecessary boolean and clarifies intent.
Suggested implementation:
// Only stat `old` when the dest exists (for the same-inode check).
// When dest doesn't exist, the old stat is unnecessary.
let same_file = match new_meta.as_ref() {
Some(new_meta) => match fs::symlink_metadata(old) {
Ok(old_meta) => super::common::same_inode(&old_meta, new_meta),
_ => false,
},
None => false,
};No additional changes are required; this preserves the "only stat old when dest exists" behavior while simplifying the control flow and clarifying intent.
| @@ -106,7 +110,42 @@ fn search_content_recursive_inner(path: &Path, depth: usize, ctx: &mut ContentSe | |||
| if ctx.cancel.load(Ordering::Relaxed) { | |||
There was a problem hiding this comment.
suggestion: The new _skip_dir_check variant duplicates most of search_content_recursive_inner; consider consolidating to reduce divergence risk.
These two functions now differ only in the initial is_dir check; the rest of the traversal logic is duplicated. This increases the risk that future changes to recursive traversal will be applied to one function but not the other. Consider factoring the common logic into a shared helper (e.g., accepting a skip_dir_check flag or moving the depth == 0 && !path.is_dir() guard to the caller) so that specialized entry points still exist but the traversal is defined in a single place.
Suggested implementation:
fn search_content_recursive_inner_skip_dir_check(
path: &Path,
depth: usize,
ctx: &mut ContentSearchContext<'_>,
) {
// Delegate to the shared traversal helper, skipping the initial is_dir check.
search_content_recursive_inner_common(path, depth, ctx, true);
}To fully implement the consolidation and avoid duplicated traversal logic, the following changes are also needed elsewhere in src/ops/search/content.rs:
-
Introduce a shared helper that contains the traversal logic and takes a flag indicating whether to skip the initial
is_dircheck, e.g.:fn search_content_recursive_inner_common( path: &Path, depth: usize, ctx: &mut ContentSearchContext<'_>, skip_dir_check: bool, ) { if ctx.cancel.load(Ordering::Relaxed) { return; } if !skip_dir_check && depth == 0 && !path.is_dir() { return; } // Move the existing recursive traversal body from // `search_content_recursive_inner` into this function. // ... }
-
Refactor the original
search_content_recursive_innerto delegate to the helper while preserving its current behavior:fn search_content_recursive_inner( path: &Path, depth: usize, ctx: &mut ContentSearchContext<'_>, ) { search_content_recursive_inner_common(path, depth, ctx, false); }
-
Remove the now-duplicated traversal code from
search_content_recursive_innerandsearch_content_recursive_inner_skip_dir_check, ensuring that all future changes to the recursive traversal are made only insearch_content_recursive_inner_common.
You will need to adapt the contents of search_content_recursive_inner_common to match the existing traversal logic currently implemented in search_content_recursive_inner, including any process_content_entry calls and depth handling that are not visible in the provided snippet.
Greptile SummaryThis PR optimizes panel refresh, filesystem metadata reads, archive handling, sorting, and recursive searches.
Confidence Score: 4/5The stale panel path index must be fixed before merging because subsequent watcher events can update or remove the wrong entry. In-place sorting changes backing-store positions while the existing path-to-position map remains populated, allowing watcher mutations to act on obsolete indices. Files Needing Attention: src/app/panel_ops.rs and src/app/types/panel.rs
|
| Filename | Overview |
|---|---|
| src/app/panel_ops.rs | Sorts the panel backing store once and derives filtered indices, but leaves the path index stale after in-place rebuild sorting. |
| src/app/types/panel.rs | Adds predicate-based filtered-index rebuilding; the method correctly rebuilds the visible view but does not repair a path index invalidated by external in-place sorting. |
| src/ops/search/content.rs | Reuses one memmem finder per scan, lazily allocates matched paths, and removes a redundant recursive directory stat without an actionable regression. |
| src/ops/search/name.rs | Defers PathBuf construction and avoids retrying failed directory metadata reads. |
| src/ops/search/pattern.rs | Adds an ASCII lowercase fast path and consolidates wildcard-star discovery into one pass. |
| src/fs/reader.rs | Reduces initial directory capacity and reuses symlink metadata for type detection. |
| src/ops/archive/tar.rs | Hoists archive-format string construction outside the listing loop. |
| src/ops/archive/zip.rs | Avoids a second path-string allocation when archive names contain no backslashes. |
Reviews (1): Last reviewed commit: "perf(ops): hot path — panel refresh, sea..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let sort_options = *panel.sort_options(); | ||
| // Re-sort the backing store in place, then rebuild the filtered view as | ||
| // indices into the now-sorted store. Avoids cloning every FileEntry. | ||
| ops::sort_entries(panel.listing.unfiltered_mut(), sort_mode, sort_options); |
There was a problem hiding this comment.
When a sort or filter rebuild reorders unfiltered_entries, path_index retains the old slot values because set_filtered_indices only rebuilds the visible view. A subsequent watcher upsert or removal therefore updates or removes the wrong file entry before the next full directory refresh.
|
Working tree clean, back at the PR commit. Here is my review. PR Review: perf(ops): panel refresh, search, fs, archive, copy hot pathsVerdict: Request changes — one correctness blocker + several minor issuesThe PR is well-researched, the rejected/deferred rationale is sound, and 🔴 Blocker:
|
…ntries rebuild_visible_entries sorts unfiltered_entries in place, but path_index (HashMap<PathBuf→usize>) kept stale pre-sort indices. ensure_index() is a no-op when non-empty, so watcher upsert/remove then mutated the wrong slot. Root-cause fix: call panel.listing.rebuild_index() after the sort so the path→index map reflects the reordered backing store. Adds regression test verifying sort + upsert-by-path lands on the correct entry (fails on master, passes with the fix). Fixes PR #111 review finding (Greptile P1 + opencode Request changes).
Review: perf(ops) — panel refresh, search, fs, archive hot pathsI independently verified the diff, ran the full CI gate, and confirmed the blocker status. The blocker flagged by earlier reviews has been fixed by the author in CI gate (all green, independently run)
Blocker: RESOLVED ✅The stale Minor issues (non-blocking, but worth a quick cleanup)
Verified correct
RecommendationApprove. The only correctness issue is resolved with a regression test, and all gates are green. Items 1–6 are quick cleanups — I'd suggest addressing at least the |
1 participant
Performance: file ops, search, archive, fs, panel refresh (PR-12)
52 findings across panel_ops, fs/*, ops/archive|batch|chunk|file_ops|helpers|natsort|search|sorting.
Fixed (19 findings)
panel_ops — refresh double-sort/clone (priority 1)
refresh_panel: sort the backing store once, derive the filtered view as indices via newPanelListing::set_filtered_indices. Eliminates the previous double-sort (once on a cloned filtered Vec, once on unfiltered) and the per-entry clone of every visibleFileEntry. (0.0.5 #4)rebuild_visible_entries: same in-place sort + index derivation, no FileEntry cloning.Cha — Copy + typ() once (priority 2)
Chais nowCopy— all fields are Copy (ChaKind, ChaMode, u64, Option, u32). Avoids Clone in hot paths. (refactor: ownership, perf, error handling, domain extraction, clippy … #5)Cha::is_dir()computestyp()once instead of callingis_dir()+is_link()(each recomputing the match on mode bits). (0.0.5 #4)Search hot path (priority 3)
contains_case_insensitive: ASCII fast path —push_str+make_ascii_lowercaseinstead ofchars().flat_map(to_lowercase)iterator for the common ASCII case. (pattern.rs 0.0.5 #4)try_simple_wildcard: single pass to collect star positions, replacing the prior count + find + rfind triple iteration. (pattern.rs refactor: ownership, perf, error handling, domain extraction, clippy … #5)memmem::Finderbuilt once per recursive scan, passed throughContentSearchContext— previously rebuilt per file (~300B Two-Way table per file). (content.rs 0.0.9 - Add async file operations, file watching, and UI workflow improvements #8)Arc<Path>allocated lazily — only on first match, not per scanned file. (content.rs refactor: ownership, perf, error handling, domain extraction, clippy … #5, search.rs refactor: ownership, perf, error handling, domain extraction, clippy … #5)search_content_recursive_inner: skips redundantpath.is_dir()stat when the caller (process_content_entry) already confirmed the type from the dirent. (search.rs Fix critical issues, enhance performance, and improve code quality #9)entry.path()(PathBuf alloc) deferred to only when the entry matched or needs recursion — skips allocation for the common non-matching dirent. (name.rs Harden file operations and improve terminal UI reliability #6)dir_meta = Some(Err(_))no longer falls through toget_file_info(which repeated the failed stat). (name.rs 0.0.5 #4, search.rs 0.0.8 - Fix dialog/popup rendering, Implement shift-selection for contiguous file ranges #7)fs/reader (priority 4)
build_file_entry: usesmetadata.is_symlink()from the already-fetchedsymlink_metadata, eliminating the redundantentry.file_type()syscall. (reader.rs 0.0.8 - Fix dialog/popup rendering, Implement shift-selection for contiguous file ranges #7)INITIAL_DIR_CAPACITY: 256 → 64 — avoids ~4.5 KiB over-allocation on small directories. (reader.rs 0.0.9 - Add async file operations, file watching, and UI workflow improvements #8)Miscellaneous
helpers::dir_size:HashSet::with_capacity(256)instead ofHashSet::new(). (helpers.rs refactor: ownership, perf, error handling, domain extraction, clippy … #5)natsort::SegData::from_slice:Box::from(s)(1 allocation) instead ofto_vec().into_boxed_slice()(2 allocations). (natsort.rs Harden file operations and improve terminal UI reliability #6)rename_entry: skipssymlink_metadata(old)when dest doesn't exist — the inode check is only needed when both exist. (entry_ops.rs Harden file operations and improve terminal UI reliability #6)list_tar:format!("{format:?}")hoisted out of the entry loop — was allocating the same string per entry. (tar.rs 0.0.8 - Fix dialog/popup rendering, Implement shift-selection for contiguous file ranges #7)add_dir_to_zip:to_string_lossy().replace(\, "/")→ single allocation — only allocates the replaced string when a backslash is present. (zip.rs 0.0.8 - Fix dialog/popup rendering, Implement shift-selection for contiguous file ranges #7, Fix critical issues, enhance performance, and improve code quality #9)Rejected / Deferred (33 findings)
Rejected — no measurable win or codebase invariant prevents it:
content.rs #6:pattern_bytes: Vec::new()when case-sensitive — zero-capacity Vec is already free (no heap alloc).model.rs #3:CloneonSearchOutcome— removing it would break existing callers that clone the outcome.model.rs #4:SearchError.message: Cow<'static, str>— error paths are cold; theString→Cowtype change ripples through every construction site for negligible gain.walk.rs #4:item_limit_reached<T>monomorphization — already deduplicated by the optimizer; the generic is on an unused param only cosmetically.batch.rs #6: unbounded progress channel → bounded — would change worker back-pressure semantics; out of scope for a perf-only PR.chunk_copy #8:copy_file_range/sendfile— Linux-only zero-copy; platform-specific syscall addition withforbid(unsafe)constraints.chunk_copy #9: per-file 256 KiB buffer → thread_local — complex lifecycle for a buffer that the allocator already reuses efficiently.copy.rs #7: reflink/COW (FICLONE/clonefile) — platform-specific syscall, forbid(unsafe).copy.rs #8: sparse-file support — requires SEEK_DATA/SEEK_HOLE, platform-specific.copy.rs #9:copy_file_to_tempextra metadata stat — test-only code path, not a production hot path.move_ops #8: doublesymlink_metadataper move — the two stats (src, dest) are structurally required for the cross-device detection; cannot be reduced without changing correctness.temp.rs #7/#8:suffixed_pathdouble allocation — runs at most 128 times per operation in the reserve loop, not per file in a batch.sorting.rs #6:NameSortKey::newallocatesBox<str>per entry — already usessort_by_cached_key(one alloc per entry, not per comparison). The 1.7M figure assumes per-comparison whichcached_keyprevents.reader.rs #9:..entry stat + owner lookup — required for correct owner/group display; caching does not eliminate the first-hit cost.common.rs #6:canonicalize_with_nearest_existing_parentO(depth) stats — structurally required for path containment validation.common.rs #7:ensure_destination_absentformat! allocation — runs once per file in batch, but the format is needed for the error message; rejecting saves nothing.natsort.rs #5:NatKey = Vec<NatKeySegment>→ smallvec — adding a dependency for a marginal gain in a path already amortized bysort_by_cached_key.helpers.rs #6:entry.path()per entry — required for the size computation; no borrow-based alternative exists forDirEntry.sevenz.rs #6:create_dir_allper entry — the TOCTOU re-verification on every entry is an intentional security invariant (documented in the code); caching the parent would reopen the symlink-swap window.zip.rs #8:fs::read_dirwithout sorting — archive entry order follows filesystem order; sorting would add cost with no functional benefit for archive creation.tar.rs #8:path_str.into_owned().into_boxed_str()always allocates — required becauseArchiveEntry.nameisBox<str>; avoiding it would need a type change toCow.pattern.rs #3: ASCII fast pathwindows().any(eq_ignore_ascii_case)O(n·m) — already an optimization over the Unicode path; the audit's suggested memmem is whatcontains_case_insensitivenow uses (fixed via the ASCII fast path above).watcher.rs #7/#8/#10/#11: Mutex hold time / O(n) scans / drain allocations — these are in the watcher event-processing path (background thread), not the interactive hot path. Restructuring the lock scope risks correctness for marginal perf.CI Gate (all pass)
No sort-order semantics changed. No new dependencies.
Cargo.lockunchanged.Summary by Sourcery
Optimize several interactive hot paths for file search, panel listing, filesystem reads, and archive operations to reduce unnecessary allocations, syscalls, and cloning while preserving behavior.
New Features:
Bug Fixes:
Enhancements:
Arc<Path>allocation lazy so it occurs only when matches are found.Tests: