Optimize rendering: precompute FileEntry fields, reduce allocations#45
Conversation
…cations - Precompute time_str, size_str, name_width in FileEntry at load time - Cache function bar labels as const [&str; 10] instead of format!() per frame - Remove .into_owned() from viewer render and search loops - Use memchr for line offset computation (~10x faster) - Cap viewer visual lines at 1M to prevent unbounded memory - ASCII fast-path for viewer search (byte-level when line is ASCII) - Fix ASCII fast-path bug: check line.is_ascii() not query.is_ascii() - Move path.clone() after guard in sync_watcher_paths - Cache hotlist/user-menu picker strings, rebuild on mutation - Fix missing rebuild_hotlist_cache() in config.rs apply_to_state() - to_string_lossy().into_owned() instead of .to_string() - NatKeySegment Vec<u8> -> Box<[u8]> to eliminate excess capacity - name_key: skip clone when name is already lowercase - format_time: Cow -> String (callers always owned) - Remove dead display_line field from FileEntry
Reviewer's GuideOptimizes rendering and search performance by precomputing per-file display fields, caching frequently built UI strings, reducing allocations and clones across panels, viewer, sort, and pickers, and adds safety and speed improvements to viewer line handling and natural sort keys. Sequence diagram for hotlist picker flow with cached stringssequenceDiagram
actor User
participant Pickers as pickers
participant AppState as state
participant Render as render
participant Dialogs as dialogs
User->>Pickers: handle_hotlist_picker(state, key, len)
alt add_current_directory
Pickers->>AppState: directory_hotlist.push(cur)
Pickers->>AppState: rebuild_hotlist_cache()
else delete_selected_directory
Pickers->>AppState: directory_hotlist.remove(picker_selected)
Pickers->>AppState: rebuild_hotlist_cache()
end
User->>Render: render_list_picker_overlay(f, area, colors, state)
Render->>AppState: read cached_hotlist_strings
Render->>Dialogs: render_list_picker_with_colors(f, title, cached_hotlist_strings, selected, hints, colors)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The cached fields on
FileEntry(time_str,size_str,name_width) are now computed in multiple places (FileEntryBuilder::build,build_file_entry, manual construction in tests,read_directoryfor..); consider centralizing this logic (e.g., a constructor/helper onFileEntry) to avoid drift if the formatting or width rules change. - The hotlist/user-menu string caches (
cached_hotlist_strings,cached_user_menu_strings) depend on callers remembering to invokerebuild_*_cache; it may be safer to encapsulate all mutations ofdirectory_hotlistanduser_menu_entriesbehind helper methods that update the caches automatically to prevent future inconsistencies. - The 1M cap on
visual_heightsin the viewer (MAX_VISUAL_LINES) clears the vector and returns early when exceeded, effectively disabling visual layout; consider either surfacing this state (e.g., a flag or message) or documenting the behavior so it’s clear how very large files are handled and that this is an intentional fallback.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The cached fields on `FileEntry` (`time_str`, `size_str`, `name_width`) are now computed in multiple places (`FileEntryBuilder::build`, `build_file_entry`, manual construction in tests, `read_directory` for `..`); consider centralizing this logic (e.g., a constructor/helper on `FileEntry`) to avoid drift if the formatting or width rules change.
- The hotlist/user-menu string caches (`cached_hotlist_strings`, `cached_user_menu_strings`) depend on callers remembering to invoke `rebuild_*_cache`; it may be safer to encapsulate all mutations of `directory_hotlist` and `user_menu_entries` behind helper methods that update the caches automatically to prevent future inconsistencies.
- The 1M cap on `visual_heights` in the viewer (`MAX_VISUAL_LINES`) clears the vector and returns early when exceeded, effectively disabling visual layout; consider either surfacing this state (e.g., a flag or message) or documenting the behavior so it’s clear how very large files are handled and that this is an intentional fallback.
## Individual Comments
### Comment 1
<location path="src/ui/viewer.rs" line_range="637-640" />
<code_context>
0
};
let width = content_width.max(1);
+ const MAX_VISUAL_LINES: usize = 1_000_000;
self.visual_heights = (0..self.line_count)
.map(|i| {
</code_context>
<issue_to_address>
**issue (bug_risk):** Clear `visual_offsets` as well when bailing out on very large files
When `line_count` exceeds `MAX_VISUAL_LINES`, we clear `visual_heights` and return, but leave `visual_offsets` unchanged. This leaves stale offsets alongside an empty heights vector, which can desynchronize viewer state and cause incorrect scrolling or even panics if callers assume both vectors stay aligned. Please also clear `visual_offsets` (and any related layout state) in this early return path to keep the structures consistent.
</issue_to_address>
### Comment 2
<location path="src/app/types.rs" line_range="611-617" />
<code_context>
self
}
pub fn build(self) -> FileEntry {
+ let time_str = format_time(self.cha.mtime().unwrap_or(std::time::UNIX_EPOCH));
+ let size_str = if self.cha.is_dir() {
+ " <DIR>".to_string()
+ } else {
+ format!("{:>10}", format_size(self.cha.len()))
+ };
+ let name_width = UnicodeWidthStr::width(self.name.as_str());
FileEntry {
name: self.name,
</code_context>
<issue_to_address>
**suggestion:** Avoid duplicating `FileEntry` formatting logic between the builder and `fs::reader`
`time_str`, `size_str`, and `name_width` are computed here in `FileEntryBuilder::build` and similarly in `fs::reader::build_file_entry`. This duplication risks the two code paths drifting (e.g., one getting formatting or width updates while the other doesn’t). Consider extracting a shared helper (such as a `FileEntry::with_cached_fields(...)` constructor or standalone function) that takes `cha` and `name` and populates these derived fields in one place.
Suggested implementation:
```rust
pub fn build(self) -> FileEntry {
let (time_str, size_str, name_width) = FileEntry::cached_fields(
self.cha.mtime().unwrap_or(std::time::UNIX_EPOCH),
self.cha.is_dir(),
self.cha.len(),
self.name.as_str(),
);
FileEntry {
name: self.name,
path: self.path,
group: self.group,
selected: self.selected,
mime_type: self.mime_type,
time_str,
size_str,
name_width,
}
}
}
impl FileEntry {
pub fn cached_fields(
mtime: std::time::SystemTime,
is_dir: bool,
len: u64,
name: &str,
) -> (String, String, usize) {
let time_str = format_time(mtime);
let size_str = if is_dir {
" <DIR>".to_string()
} else {
format!("{:>10}", format_size(len))
};
let name_width = UnicodeWidthStr::width(name);
(time_str, size_str, name_width)
}
}
```
To fully remove the duplication you mentioned, you should also update `fs::reader::build_file_entry` (or any other place that manually computes `time_str`, `size_str`, and `name_width`) to call:
```rust
let (time_str, size_str, name_width) = FileEntry::cached_fields(
cha.mtime().unwrap_or(std::time::UNIX_EPOCH),
cha.is_dir(),
cha.len(),
name.as_str(),
);
```
and then use these values instead of recomputing them locally.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request introduces several performance optimizations and structural improvements, such as pre-calculating display strings and widths for FileEntry, implementing string caching for the hotlist and user menu, and utilizing the memchr crate for faster line offset computations. Additionally, NatKeySegment was refactored to use boxed slices for better memory efficiency, and a safety limit was added to visual height calculations in the viewer. Feedback from the reviewer identifies critical compilation errors where struct fields were incorrectly accessed as methods in src/app/types.rs and src/fs/reader.rs. There is also a concern regarding an optimization in the sorting logic that alters tie-breaking behavior for case-insensitive matches, potentially affecting the expected sort order.
…menu mutations, fix viewer cap cleanup
Summary by Sourcery
Optimize UI rendering and viewer performance by precomputing and caching frequently used values, while tightening memory usage and synchronization behavior.
Bug Fixes:
Enhancements:
Build:
Tests: