perf(ui): panels, viewer, dialogs render path - #110
Conversation
Audit PR-13 (35 performance findings in the UI render path). Fixed (21 findings): - panels/mod.rs: cache dirs::home_dir() thread-locally; indexed access replaces skip() iterator; ChaMode written straight into buffers instead of display_permissions_raw String alloc per row; format_size replaced by inline write_size; status bar reuses cached entry.size_str and eliminates scratch.clone(); with_capacity sized for CJK (4× width). - viewer/open.rs: "<DIR>" borrowed instead of .to_string() per dir entry; Vec::with_capacity starts at READ_CHUNK not up to 100MB. - viewer/toggle.rs: text metrics computed once then reused (line_offsets non-empty guard); grouped borrow_mut in invalidate_visual_cache. - viewer/search.rs: decode_lossy_with_map reserves 3× for U+FFFD expansion. - viewer/hex.rs: push_byte_hex uses stack array + push_str (1 write, not 3). - input/normal.rs: Alt+Backspace avoids prev_path.clone(). - input/menu_actions.rs: hotlist path bound once before .any() scan. - dialogs/archive.rs: removed redundant INPUT_BUF (compute_visible_window already returns owned String); render_button_row reserves 2n capacity. - dialogs/input.rs: documented single-active-field SegmentCache assumption. - dialogs/list_picker.rs: hoisted dialog_with_colors to a single let. - render.rs: icon_theme computed only on the panel-layout path, not Viewing. - theme.rs: eq_ignore_ascii_case replaces to_ascii_lowercase() allocation. Rejected (14 findings) with justification in PR body. cargo fmt + clippy -D warnings + test (302 pass) + release build all green.
Reviewer's GuideThis PR focuses on reducing allocations and per-frame work in the panels, viewer, dialogs, search, and theme render paths, mainly by caching expensive values, reusing buffers, and avoiding unnecessary String creation while preserving behavior and UI semantics. Sequence diagram for viewer mode toggle text metrics cachingsequenceDiagram
participant ViewerState
participant RenderCache
ViewerState->>ViewerState: toggle_view_mode()
ViewerState->>ViewerState: invalidate_visual_cache()
alt [view_mode == Text && originally_binary]
opt [line_offsets.is_empty()]
ViewerState->>ViewerState: compute_text_metrics(raw_bytes)
ViewerState->>ViewerState: set line_offsets, line_count, max_line_width
ViewerState->>RenderCache: cached_line_num_col_width.set(line_number_column_width(line_count))
end
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 2 issues, and left some high level feedback:
- The new
write_sizehelper duplicates the logic offormat_size; consider refactoring so they share a single implementation to avoid divergence in formatting behavior over time. - In
push_byte_hex, thefrom_utf8call is guaranteed to succeed given the fixed ASCII data, so you can avoid theResulthandling (e.g., withfrom_utf8_uncheckedor a precomputed table) to reduce overhead on the hex render hot path.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `write_size` helper duplicates the logic of `format_size`; consider refactoring so they share a single implementation to avoid divergence in formatting behavior over time.
- In `push_byte_hex`, the `from_utf8` call is guaranteed to succeed given the fixed ASCII data, so you can avoid the `Result` handling (e.g., with `from_utf8_unchecked` or a precomputed table) to reduce overhead on the hex render hot path.
## Individual Comments
### Comment 1
<location path="src/ui/viewer/hex.rs" line_range="22-35" />
<code_context>
- buf.push(' ');
+ // Build into a 3-byte stack array, then push in one shot — avoids three
+ // separate push calls (amortized check + len update) per byte.
+ let trio = [
+ HEX_CHARS[(b >> NIBBLE_BITS) as usize],
+ HEX_CHARS[(b & LOW_NIBBLE_MASK) as usize],
+ b' ',
+ ];
+ // `trio` is built solely from `HEX_CHARS` (ASCII) and a space, so it is
+ // always valid UTF-8. `from_utf8` never fails here.
+ if let Ok(s) = std::str::from_utf8(&trio) {
+ buf.push_str(s);
+ }
}
</code_context>
<issue_to_address>
**suggestion (performance):** The `from_utf8` check in `push_byte_hex` is unnecessary and adds overhead; consider a cheaper path.
Since `trio` is built only from `HEX_CHARS` (0–9, a–f) and a space, it’s statically guaranteed to be valid UTF-8, so `from_utf8` can never fail. Calling it in this per-byte hot loop adds an unnecessary branch and error path. To keep the single `push_str` optimization without the extra check, either retain the original per-char `push` calls or use `unsafe { std::str::from_utf8_unchecked(&trio) }` with a brief comment documenting this invariant.
```suggestion
fn push_byte_hex(buf: &mut String, b: u8) {
// Build into a 3-byte stack array, then push in one shot — avoids three
// separate push calls (amortized check + len update) per byte.
let trio = [
HEX_CHARS[(b >> NIBBLE_BITS) as usize],
HEX_CHARS[(b & LOW_NIBBLE_MASK) as usize],
b' ',
];
// `trio` is built solely from `HEX_CHARS` (ASCII '0'–'9', 'a'–'f') and a space,
// so it is statically guaranteed to be valid UTF-8. This makes the unchecked
// conversion safe and avoids a per-byte branch in this hot loop.
let s = unsafe { std::str::from_utf8_unchecked(&trio) };
buf.push_str(s);
}
```
</issue_to_address>
### Comment 2
<location path="src/ui/panels/tests.rs" line_range="56" />
<code_context>
+fn entry_line(entry: &FileEntry, width: usize, show_permissions: bool) -> String {
</code_context>
<issue_to_address>
**suggestion (testing):** Extend `entry_line`-based tests to cover CJK/wide-character content to validate the new capacity sizing.
The new `format_entry_line` implementation allocates with `String::with_capacity(content_width.saturating_mul(4) + 8)` to support CJK/wide characters, but current tests only cover narrow content. Please add `entry_line`-based cases with double-width filenames/suffixes (e.g., Japanese/Chinese text, combining marks) to verify lines render fully, truncate correctly, and never panic, so the updated sizing is validated and protected against future regressions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| fn push_byte_hex(buf: &mut String, b: u8) { | ||
| buf.push(HEX_CHARS[(b >> NIBBLE_BITS) as usize] as char); | ||
| buf.push(HEX_CHARS[(b & LOW_NIBBLE_MASK) as usize] as char); | ||
| buf.push(' '); | ||
| // Build into a 3-byte stack array, then push in one shot — avoids three | ||
| // separate push calls (amortized check + len update) per byte. | ||
| let trio = [ | ||
| HEX_CHARS[(b >> NIBBLE_BITS) as usize], | ||
| HEX_CHARS[(b & LOW_NIBBLE_MASK) as usize], | ||
| b' ', | ||
| ]; | ||
| // `trio` is built solely from `HEX_CHARS` (ASCII) and a space, so it is | ||
| // always valid UTF-8. `from_utf8` never fails here. | ||
| if let Ok(s) = std::str::from_utf8(&trio) { | ||
| buf.push_str(s); | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (performance): The from_utf8 check in push_byte_hex is unnecessary and adds overhead; consider a cheaper path.
Since trio is built only from HEX_CHARS (0–9, a–f) and a space, it’s statically guaranteed to be valid UTF-8, so from_utf8 can never fail. Calling it in this per-byte hot loop adds an unnecessary branch and error path. To keep the single push_str optimization without the extra check, either retain the original per-char push calls or use unsafe { std::str::from_utf8_unchecked(&trio) } with a brief comment documenting this invariant.
| fn push_byte_hex(buf: &mut String, b: u8) { | |
| buf.push(HEX_CHARS[(b >> NIBBLE_BITS) as usize] as char); | |
| buf.push(HEX_CHARS[(b & LOW_NIBBLE_MASK) as usize] as char); | |
| buf.push(' '); | |
| // Build into a 3-byte stack array, then push in one shot — avoids three | |
| // separate push calls (amortized check + len update) per byte. | |
| let trio = [ | |
| HEX_CHARS[(b >> NIBBLE_BITS) as usize], | |
| HEX_CHARS[(b & LOW_NIBBLE_MASK) as usize], | |
| b' ', | |
| ]; | |
| // `trio` is built solely from `HEX_CHARS` (ASCII) and a space, so it is | |
| // always valid UTF-8. `from_utf8` never fails here. | |
| if let Ok(s) = std::str::from_utf8(&trio) { | |
| buf.push_str(s); | |
| } | |
| } | |
| fn push_byte_hex(buf: &mut String, b: u8) { | |
| // Build into a 3-byte stack array, then push in one shot — avoids three | |
| // separate push calls (amortized check + len update) per byte. | |
| let trio = [ | |
| HEX_CHARS[(b >> NIBBLE_BITS) as usize], | |
| HEX_CHARS[(b & LOW_NIBBLE_MASK) as usize], | |
| b' ', | |
| ]; | |
| // `trio` is built solely from `HEX_CHARS` (ASCII '0'–'9', 'a'–'f') and a space, | |
| // so it is statically guaranteed to be valid UTF-8. This makes the unchecked | |
| // conversion safe and avoids a per-byte branch in this hot loop. | |
| let s = unsafe { std::str::from_utf8_unchecked(&trio) }; | |
| buf.push_str(s); | |
| } |
| &mut suffix, | ||
| &mut out, | ||
| ); | ||
| let _ = &suffix; // scratch buffer required by format_entry_line, not asserted |
There was a problem hiding this comment.
suggestion (testing): Extend entry_line-based tests to cover CJK/wide-character content to validate the new capacity sizing.
The new format_entry_line implementation allocates with String::with_capacity(content_width.saturating_mul(4) + 8) to support CJK/wide characters, but current tests only cover narrow content. Please add entry_line-based cases with double-width filenames/suffixes (e.g., Japanese/Chinese text, combining marks) to verify lines render fully, truncate correctly, and never panic, so the updated sizing is validated and protected against future regressions.
Greptile SummaryPerformance-focused render-path optimizations preserve existing behavior while reducing repeated allocation and computation.
Confidence Score: 5/5The PR appears safe to merge, with no concrete behavioral, security, or build regressions identified. The changed paths retain the prior rendering, formatting, navigation, and viewer-state semantics while reducing allocations and redundant computation.
|
| Filename | Overview |
|---|---|
| src/ui/panels/mod.rs | Optimizes visible-entry traversal, metadata formatting, home-directory lookup, and status-bar buffers without changing established output contracts. |
| src/ui/viewer/toggle.rs | Reuses immutable text metrics across mode toggles and safely groups mutations of distinct render-cache cells. |
| src/ui/viewer/open.rs | Reduces initial read allocation and avoids allocating directory-size labels while preserving viewer content and truncation behavior. |
| src/ui/viewer/search.rs | Correctly reserves for worst-case UTF-8 replacement expansion without changing decoded text or byte mappings. |
| src/ui/dialogs/archive.rs | Renders the already-owned visible input window directly and improves button-span capacity sizing. |
| src/input/normal.rs | Moves the history path into panel state and safely reads it back after refresh, which does not alter the panel path. |
| src/render.rs | Defers icon-theme lookup until the panel-rendering path where it is actually required. |
| src/ui/theme.rs | Replaces allocating lowercase conversion with equivalent ASCII-insensitive comparisons. |
Reviews (1): Last reviewed commit: "perf(ui): render path — panels, viewer, ..." | Re-trigger Greptile
|
Now I have a thorough understanding of all the changes. Let me compile my review. PR Review:
|
| Change | Verification |
|---|---|
ChaMode::new() via write! replaces display_permissions_raw |
display_permissions_raw is literally ChaMode::new(mode).to_string() — byte-identical output. PERMS_WIDTH = 9 is correct (3×3 ASCII chars). |
line_offsets.is_empty() guard in toggle.rs |
compute_line_offsets always returns vec![0] minimum — never empty after first call. Safe. |
write_size mirrors format_size |
Logic, constants (1024.0), units, and format specifiers all match exactly. |
filtered_get(i) loop replaces skip().take() |
Genuinely O(1) per row vs O(start_idx) walk; handles dead indices via let-else continue. |
OnceCell home_dir cache |
Correct; to_string_lossy() per-call is no regression (original did the same). |
Cow::Borrowed("<DIR>") in viewer listing |
Correct — avoids per-dir-entry allocation. |
eq_ignore_ascii_case in theme.rs |
Correct — avoids to_ascii_lowercase() allocation, identical matching logic. |
Thread-local borrow grouping in toggle.rs |
Correct — three RefCell borrows grouped into one scope. |
The documentation comments (especially the SegmentCache single-active-field assumption in input.rs) are valuable additions.
The perf optimization in 17aa022 reused entry.size_str in the status bar to avoid a per-frame format_size alloc, but size_str is the column-padded cached form ({:>10} for files, " <DIR>" for dirs) — leaking leading spaces into the status metadata (" 1.0 KB | ..."), a visual regression vs the old unpadded format_size(entry.size()). Fix: trim_start() the cached string — format_size output never carries leading whitespace, so this recovers the unpadded display form with zero allocation. Added a render test pinning the unpadded form.
|
I've completed a thorough review. CI is green (fmt, clippy, 302 tests, release build all pass). Here's my analysis. Review:
|
| Change | Check |
|---|---|
filtered_get(i) loop replaces skip().take() |
O(1)/row vs O(start_idx); dead indices handled via let-else continue (panels/mod.rs:211) |
line_offsets.is_empty() guard |
Safe — compute_line_offsets always returns vec![0] minimum (open.rs:88) |
normal.rs path read-back after set_path |
Correct — refresh_panel never mutates panel.path() in either branch (panel_ops.rs:33) |
theme.rs eq_ignore_ascii_case |
Equivalent for the all-ASCII config values |
toggle.rs borrow grouping / invalidate_visual_cache |
Correct — three RefCell borrows in one scope |
archive.rs INPUT_BUF removal |
Correct — window.text rendered directly, no copy |
The documentation additions (especially the SegmentCache single-active-field assumption) are valuable. Net: request changes for the directory status-bar bug; the rest are optional polish.
199daef trimmed entry.size_str to recover the unpadded form, but for directories size_str is the column-cache placeholder " <DIR>", not a size — so the status bar showed "<DIR>" instead of the directory's byte size that the pre-perf code produced via format_size(entry.size()). Format the entry size directly into a scratch buffer via the existing zero-alloc write_size helper, for every entry. Files keep their unpadded human size; directories show the real byte size again. Tests: extend the status-bar coverage with a directory case asserting the bar shows a size (unit suffix) and never the <DIR> placeholder.
|
Clippy is clean and all regression tests pass. Here is my review. Review:
|
| Change | Verification |
|---|---|
ChaMode::new(mode) via write! replaces display_permissions_raw |
display_permissions_raw is literally ChaMode::new(mode).to_string() (file_entry.rs:267-270). PERM_TRIPLES has exactly 3 entries × 3 ASCII chars → width is always 9, so the hardcoded PERMS_WIDTH = 9 is correct (special bits like setuid substitute single chars s/S/t, width unchanged). |
line_offsets.is_empty() toggle guard (toggle.rs:69) |
compute_line_offsets always returns ≥ vec![0] (open.rs:88-91); for originally_binary files build initializes line_offsets to Vec::new() (open.rs:237), so the guard computes once then reuses. Safe. |
write_size mirrors format_size |
Logic, constants, rounding, and format specifiers are identical. |
normal.rs path read-back (normal.rs:583) |
set_path stores prev_path into panel.path; refresh_panel never mutates panel.path on either branch; pop_history only touches the history deque. Read-back is equivalent to the old clone. |
filtered_get(i) loop (panels/mod.rs:211) |
O(1)/row vs old O(start_idx) walk; dead indices handled by let-else continue. |
theme.rs eq_ignore_ascii_case |
Equivalent for the all-ASCII config token set. |
toggle.rs borrow grouping, archive.rs INPUT_BUF removal, Cow::Borrowed("<DIR>"), render.rs icon-theme deferral |
All correct. |
CI confirms: clippy -D warnings clean, 3/3 status-bar regression tests pass.
Optional polish (non-blocking)
-
write_sizeduplicatesformat_size(panels/mod.rs:518vsfile_entry.rs:82). Byte-identical today, but two copies can drift. Recommend makingformat_sizewrapwrite_sizeso there's one source of truth. (Also flagged by Sourcery.) -
open.rs:357Vec reservation is a pessimization on the common path.READ_CHUNK.min(file_size)(64 KB) forces ~8 reallocations for any file between 64 KB and 100 MB, where the oldfile_size.min(MAX_VIEW_SIZE + 1)reserved exactly once. Suggestfile_size.min(MAX_VIEW_SIZE)— exact for the common case, still capped for the extreme case. -
menu_actions.rs:94now allocatesto_path_buf()unconditionally (even when the path is already hotlisted). Negligible (menu action, not per-frame), but the change isn't clearly an improvement over the old lazy allocation. -
Sourcery's
unsafe { from_utf8_unchecked }suggestion forhex.rsshould be rejected — it violates the project's hard ruleunsafe_code = "forbid". The currentif let Ok(s) = from_utf8(&trio)is the correct approach under that constraint; the silent drop is acceptable since failure is statically impossible.
None of these block merge. Nice work on the documentation comments (especially the SegmentCache single-active-field assumption in input.rs).
1 participant
Audit PR-13: Performance — viewer, panels, dialogs, theme, render path
35 findings. 21 fixed, 14 rejected (justification below).
Fixed
panels/mod.rs#2with_capacity(content_width+8)undersizes for CJKcontent_width * 4 + 8panels/mod.rs#4dirs::home_dir()every frameOnceCellpanels/mod.rs#5display_permissions_rawallocates per visible rowChaModewritten directly into buffer viawrite!; width is constant 9panels/mod.rs#7format_size(entry.size())duplicates cachedsize_strentry.size_strfieldpanels/mod.rs#8format_size(panel.selected_size())per framewrite_sizeformats inline without String allocpanels/mod.rs#9scratch.clone()every status-bar rendersummaryandmetabuffers, no clonepanels/mod.rs#10skip(start_idx)walks from zero each framefiltered_get(i)instart_idx..end_idxrangeviewer/open.rs#7"<DIR>".to_string()per dir entryCow::Borrowed("<DIR>")viewer/open.rs#8Vec::with_capacityup to 100MBREAD_CHUNK, grow naturallyviewer/toggle.rs#4borrow_mut()viewer/toggle.rs#5compute_text_metricsO(n) on every toggleline_offsets.is_empty()— computed once, reusedviewer/search.rs#2with_capacity(slice.len())undersizes for U+FFFDslice.len() * 3viewer/hex.rs#2push_byte_hex3xbuf.push()per bytepush_strinput/normal.rs#7prev_path.clone()on Alt+Backspaceset_path(no clone)input/menu_actions.rs#1.any()dialogs/archive.rs#3INPUT_BUFdoesn't save allocationswindow.textdirectlydialogs/archive.rs#4Vec::with_capacity(buttons.len())under-reservesbuttons.len() * 2for 2n-1 spansdialogs/input.rs#2SegmentCacheshared by all fieldsdialogs/list_picker.rs#3dialog_with_colorscomputed twiceletrender.rs#5icon_theme()computed on Viewing paththeme.rs#3to_ascii_lowercase()allocates Stringeq_ignore_ascii_caseRejected
panels/mod.rs#3outUnicodeWidthStr::widthcall on a short string; accumulation adds complexity for negligible gainpanels/mod.rs#6format!(" Error: {err}")per framerender_dialog_map.rs#2ArchiveExtractper-frameformat!PathBufdisplay + entry count; correct caching requires persistent state, violating the pure-render contract. Dialog is transient.dialogs/mod.rs#3DialogKindderivesCloneDialogKindis never cloned on the render path — constructed fresh each frame. Removing the derive risks breakage for zero benefit.input/normal.rs#8build_help_message().to_string()per F1build_help_messagealready OnceLock-caches as&'static str. The.to_string()is one alloc per keypress, not per frame. ChangingDialogKind::Helpto store&'static strchurns the type for negligible gain.mode_dispatch.rs#2dispatch_viewer_keymatch per keypressOptionper keypress — structural dispatch, not hot loop.viewer/loader.rs#3wait-timeoutcrate (new dep) for a non-UI-blocking path.viewer/tests.rs#4yield_nowviewer/open.rs#9compute_line_offsets+compute_max_line_width+ UTF-8 validation.viewer/open.rs#10from_utf8full-buffer scan for boolshould_open_as_textonly covers firstNUL_BYTE_SCAN_LIMITbytes; full UTF-8 check still needed forhas_invalid_utf8warning.menu.rs#2dropdown_item_max_widthsOnceLockOnceLock, computed once. Audit marks "acceptable".menu.rs#3render_padded_text3xset_stringnpanels/tests.rs#3entry_lineallocates unusedsuffixsuffixis a required parameter offormat_entry_line(scratch buffer). Added clarifying comment.dialogs/archive.rs#2Line::from(vec![...])for 2 spansratatui::Linedoes not implFrom<[Span; N]>—Vec<Span>is the only option.CI
cargo fmt✅cargo clippy --locked --all-targets -- -D warnings✅cargo test --locked✅ (302 passed)cargo build --release --locked✅Summary by Sourcery
Tune UI render paths for panels, viewer, dialogs, and theming to reduce allocations and per-frame work while preserving behavior.
Enhancements:
Tests: