mirror of
https://github.com/lotabout/skim.git
synced 2026-09-10 07:16:23 -04:00
chore: unify filter mode & squeeze more perf (#974)
* refactor: move filter mode into run_with via should_enter Remove the standalone `filter` function from main.rs and integrate filter mode into the main `run_with` pipeline. When `options.filter` is set, `should_enter` now waits for all items to be processed and returns false (skipping TUI), and `App::results` returns all matched items. This unifies filter mode with the rest of the codebase so it benefits from all other flags (sorting, tiebreaks, etc.). https://claude.ai/code/session_01T7pa2RRX85MvBtnH7PWtmw * perf: optimize filter mode to match single-pass performance Three changes that eliminate the performance regression from routing filter mode through run_with: 1. should_enter(): Wait for reader to finish BEFORE starting matcher, then run matcher exactly once. The old polling loop called restart_matcher() repeatedly, each time resetting the ItemPool taken counter and re-processing all items from scratch. With 1M items arriving in batches, items were matched multiple times. 2. matcher.run(): Remove unnecessary .enumerate() (index was discarded) and remove item.clone() — into_par_iter() yields owned values so the Arc can be moved directly into MatchedItem. 3. App::results(): In filter mode, drain items instead of cloning to avoid 271K MatchedItem clones + Arc allocations. Benchmark (1M file paths, query "test", 10 runs): Old standalone filter: 5.306s ± 0.085s New unified filter: 4.932s ± 0.099s (1.08x faster) https://claude.ai/code/session_01T7pa2RRX85MvBtnH7PWtmw * perf: make restart_matcher incremental when force=false When force=false, skip the item pool reset so take() only returns new (untaken) items. The callback merges new sorted matches into the existing sorted result list using an O(n+m) merge, instead of replacing it. This fixes the root cause: previously every restart_matcher call re-processed ALL items from scratch because reset() set the taken counter to 0. This benefits all polling callers (filter, select-1, exit-0, sync), not just filter mode. Items arriving in batches are now each matched exactly once, and matching overlaps with I/O since batches are processed as they arrive. When force=true (query changed), behavior is unchanged: full reset and re-match. Reverts the filter-specific workaround from the previous commit in favor of this general fix; the original polling loop in should_enter is now efficient. Benchmark (1M file paths, query "test", 10 runs): Old standalone filter (master): 4.566s ± 0.055s Incremental restart_matcher: 3.654s ± 0.035s (1.25x faster) https://claude.ai/code/session_01T7pa2RRX85MvBtnH7PWtmw * refactor: extract sorted_merge into MatchedItem method Move the two-sorted-list merge from the restart_matcher closure into MatchedItem::sorted_merge() for clarity and reusability. The method merges two Vec<MatchedItem> lists that are already sorted by rank into a single sorted Vec in O(n+m) time. https://claude.ai/code/session_01T7pa2RRX85MvBtnH7PWtmw * fix: preserve incremental matches across render cycles When restart_matcher runs with force=false, the callback marks results with a MergeStrategy so the render loop knows how to combine them with existing items. Previously, render's .take() would drain processed_items to None, and the next incremental callback would create a fresh batch — causing the render to replace all items with just the latest batch, losing previously matched items. Now: - Replace: full re-match (force=true), replaces item list entirely - SortedMerge: incremental sorted results, merged into item_list.items - Append: incremental unsorted results (--no-sort), appended This fixes match count consistency in interactive mode. With 1M items and query "test", match count is now 290,083 on every run (matching fzf's consistency), vs wildly varying counts before (min 13, max 56,950). https://claude.ai/code/session_01T7pa2RRX85MvBtnH7PWtmw * chore: fmt & clippy --------- Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
parent
ad91558749
commit
4147eccb94
|
|
@ -128,7 +128,6 @@ fn sk_main(mut opts: SkimOptions) -> Result<i32> {
|
|||
let history_file = opts.history_file.clone();
|
||||
//------------------------------------------------------------------------------
|
||||
let bin_options = BinOptions {
|
||||
filter: opts.filter.clone(),
|
||||
print_query: opts.print_query,
|
||||
print_cmd: opts.print_cmd,
|
||||
print_score: opts.print_score,
|
||||
|
|
@ -150,10 +149,6 @@ fn sk_main(mut opts: SkimOptions) -> Result<i32> {
|
|||
let rx_item = cmd_collector.borrow().of_bufread(BufReader::new(std::io::stdin()));
|
||||
Some(rx_item)
|
||||
};
|
||||
// filter mode
|
||||
if opts.filter.is_some() {
|
||||
return Ok(filter(&bin_options, &opts, rx_item));
|
||||
}
|
||||
Some(Skim::run_with(opts, rx_item)?)
|
||||
}) else {
|
||||
return Ok(135);
|
||||
|
|
@ -243,7 +238,6 @@ fn write_history_to_file(
|
|||
#[derive(Builder)]
|
||||
#[allow(missing_docs)]
|
||||
pub struct BinOptions {
|
||||
filter: Option<String>,
|
||||
output_ending: String,
|
||||
print_query: bool,
|
||||
print_cmd: bool,
|
||||
|
|
@ -251,63 +245,3 @@ pub struct BinOptions {
|
|||
print_header: bool,
|
||||
strip_ansi: bool,
|
||||
}
|
||||
|
||||
/// Runs skim in filter mode, matching items against a fixed query without interactive UI
|
||||
pub fn filter(bin_option: &BinOptions, options: &SkimOptions, source: Option<SkimItemReceiver>) -> i32 {
|
||||
use skim::matcher::Matcher;
|
||||
|
||||
let default_command = match env::var("SKIM_DEFAULT_COMMAND").as_ref().map(String::as_ref) {
|
||||
Ok("") | Err(_) => "find .".to_owned(),
|
||||
Ok(val) => val.to_owned(),
|
||||
};
|
||||
let query = bin_option.filter.clone().unwrap_or_default();
|
||||
let cmd = options.cmd.clone().unwrap_or(default_command);
|
||||
|
||||
// output query
|
||||
if bin_option.print_query {
|
||||
print!("{}{}", query, bin_option.output_ending);
|
||||
}
|
||||
|
||||
if bin_option.print_cmd {
|
||||
print!("{}{}", cmd, bin_option.output_ending);
|
||||
}
|
||||
|
||||
//------------------------------------------------------------------------------
|
||||
// matcher - use the unified factory creation from Matcher
|
||||
let engine_factory = Matcher::create_engine_factory(options);
|
||||
let engine = engine_factory.create_engine_with_case(&query, options.case);
|
||||
|
||||
//------------------------------------------------------------------------------
|
||||
// start
|
||||
let components_to_stop = Arc::new(AtomicUsize::new(0));
|
||||
|
||||
let stream_of_item = source.unwrap_or_else(|| {
|
||||
let (ret, _control) = options.cmd_collector.borrow_mut().invoke(&cmd, components_to_stop);
|
||||
ret
|
||||
});
|
||||
|
||||
let mut num_matched = 0;
|
||||
let mut stdout_lock = std::io::stdout().lock();
|
||||
let mut items = Vec::new();
|
||||
|
||||
// Collect all items from the stream until the channel is closed
|
||||
while let Ok(batch) = stream_of_item.recv() {
|
||||
items.extend(batch);
|
||||
}
|
||||
|
||||
let mut matched_items: Vec<_> = items
|
||||
.iter()
|
||||
.filter_map(|item| engine.match_item(item.as_ref()).map(|result| (item, result)))
|
||||
.collect();
|
||||
|
||||
if options.tac {
|
||||
matched_items.reverse();
|
||||
}
|
||||
|
||||
matched_items.iter().for_each(|(item, _match_result)| {
|
||||
num_matched += 1;
|
||||
let _ = write!(stdout_lock, "{}{}", item.output(), bin_option.output_ending);
|
||||
});
|
||||
|
||||
i32::from(num_matched == 0)
|
||||
}
|
||||
|
|
|
|||
28
src/item.rs
28
src/item.rs
|
|
@ -109,7 +109,33 @@ impl Deref for MatchedItem {
|
|||
}
|
||||
}
|
||||
|
||||
impl MatchedItem {}
|
||||
impl MatchedItem {
|
||||
/// Merge two sorted `Vec<MatchedItem>` lists into one, preserving sort order by rank.
|
||||
///
|
||||
/// Both input lists must already be sorted by `rank` (ascending). The merge is O(n+m).
|
||||
pub fn sorted_merge(existing: Vec<MatchedItem>, incoming: Vec<MatchedItem>) -> Vec<MatchedItem> {
|
||||
if existing.is_empty() {
|
||||
return incoming;
|
||||
}
|
||||
if incoming.is_empty() {
|
||||
return existing;
|
||||
}
|
||||
|
||||
let mut merged = Vec::with_capacity(existing.len() + incoming.len());
|
||||
let mut a = existing.into_iter().peekable();
|
||||
let mut b = incoming.into_iter().peekable();
|
||||
while a.peek().is_some() && b.peek().is_some() {
|
||||
if a.peek().unwrap().rank <= b.peek().unwrap().rank {
|
||||
merged.push(a.next().unwrap());
|
||||
} else {
|
||||
merged.push(b.next().unwrap());
|
||||
}
|
||||
}
|
||||
merged.extend(a);
|
||||
merged.extend(b);
|
||||
merged
|
||||
}
|
||||
}
|
||||
|
||||
use std::cmp::Ordering as CmpOrd;
|
||||
|
||||
|
|
|
|||
28
src/lib.rs
28
src/lib.rs
|
|
@ -369,8 +369,14 @@ impl Skim {
|
|||
/// # Panics
|
||||
///
|
||||
/// Panics if the tui fails to initilize
|
||||
pub fn run_with(options: SkimOptions, source: Option<SkimItemReceiver>) -> Result<SkimOutput> {
|
||||
pub fn run_with(mut options: SkimOptions, source: Option<SkimItemReceiver>) -> Result<SkimOutput> {
|
||||
trace!("running skim");
|
||||
// In filter mode, use the filter string as the query for matching
|
||||
if let Some(ref filter_query) = options.filter
|
||||
&& options.query.is_none()
|
||||
{
|
||||
options.query = Some(filter_query.clone());
|
||||
}
|
||||
let mut skim = Self::init(options, source)?;
|
||||
|
||||
skim.start();
|
||||
|
|
@ -583,13 +589,31 @@ where
|
|||
.enter()
|
||||
}
|
||||
|
||||
/// Checks read-0 select-1, and sync to wait and returns whether or not we should enter
|
||||
/// Checks read-0 select-1, filter, and sync to wait and returns whether or not we should enter
|
||||
fn should_enter(&mut self) -> bool {
|
||||
let reader_control = self
|
||||
.reader_control
|
||||
.as_ref()
|
||||
.expect("reader_control needs to be initilized using Skim::start");
|
||||
let app = &mut self.app;
|
||||
|
||||
// Filter mode: wait for all items to be read and matched, then return without entering TUI
|
||||
if app.options.filter.is_some() {
|
||||
trace!("filter mode: waiting for all items to be processed");
|
||||
loop {
|
||||
let matcher_stopped = app.matcher_control.stopped();
|
||||
let reader_done = reader_control.is_done();
|
||||
if matcher_stopped && reader_done && app.item_pool.num_not_taken() == 0 {
|
||||
break;
|
||||
}
|
||||
std::thread::sleep(Duration::from_millis(1));
|
||||
app.restart_matcher(false);
|
||||
}
|
||||
app.item_list.items = app.item_list.processed_items.lock().take().unwrap_or_default().items;
|
||||
debug!("filter mode: matched {} items", app.item_list.items.len());
|
||||
return false;
|
||||
}
|
||||
|
||||
// Deal with read-0 / select-1
|
||||
let min_items_before_enter = if app.options.exit_0 {
|
||||
1
|
||||
|
|
|
|||
|
|
@ -1148,8 +1148,11 @@ impl App {
|
|||
}
|
||||
|
||||
/// Returns the selected items as results
|
||||
pub fn results(&self) -> Vec<Arc<MatchedItem>> {
|
||||
if self.options.multi && !self.item_list.selection.is_empty() {
|
||||
pub fn results(&mut self) -> Vec<Arc<MatchedItem>> {
|
||||
if self.options.filter.is_some() {
|
||||
// In filter mode, drain items to avoid cloning
|
||||
self.item_list.items.drain(..).map(Arc::new).collect()
|
||||
} else if self.options.multi && !self.item_list.selection.is_empty() {
|
||||
self.item_list
|
||||
.selection
|
||||
.iter()
|
||||
|
|
@ -1186,7 +1189,7 @@ impl App {
|
|||
|
||||
let matcher_stopped = self.matcher_control.stopped();
|
||||
if force || (matcher_stopped && self.item_pool.num_not_taken() > 0) {
|
||||
trace!("restarting matcher");
|
||||
trace!("restarting matcher, force={force}");
|
||||
// Reset debounce timer on any restart to prevent interference
|
||||
self.last_matcher_restart = std::time::Instant::now();
|
||||
self.pending_matcher_restart = false;
|
||||
|
|
@ -1207,15 +1210,47 @@ impl App {
|
|||
let processed_items = self.item_list.processed_items.clone();
|
||||
let no_sort = self.options.no_sort;
|
||||
|
||||
self.item_pool.reset();
|
||||
if force {
|
||||
self.item_pool.reset();
|
||||
}
|
||||
|
||||
self.matcher_control = self.matcher.run(query, item_pool, thread_pool, move |mut matches| {
|
||||
debug!("Got {} results from matcher, sending to item list...", matches.len());
|
||||
|
||||
// Send matched items directly (header_lines are now handled by the Header widget)
|
||||
if !no_sort {
|
||||
matches.sort_by_key(|item| item.rank);
|
||||
}
|
||||
*processed_items.lock() = Some(crate::tui::item_list::ProcessedItems { items: matches });
|
||||
|
||||
use crate::tui::item_list::{MergeStrategy, ProcessedItems};
|
||||
if force {
|
||||
// Full re-match: replace all results
|
||||
*processed_items.lock() = Some(ProcessedItems {
|
||||
items: matches,
|
||||
merge: MergeStrategy::Replace,
|
||||
});
|
||||
} else {
|
||||
// Incremental: merge new matches into any unconsumed processed items,
|
||||
// and mark with merge strategy so the render loop merges with item_list.items
|
||||
let merge_strategy = if no_sort {
|
||||
MergeStrategy::Append
|
||||
} else {
|
||||
MergeStrategy::SortedMerge
|
||||
};
|
||||
let mut guard = processed_items.lock();
|
||||
if let Some(ref mut existing) = *guard {
|
||||
if no_sort {
|
||||
existing.items.extend(matches);
|
||||
} else {
|
||||
let old = std::mem::take(&mut existing.items);
|
||||
existing.items = MatchedItem::sorted_merge(old, matches);
|
||||
}
|
||||
} else {
|
||||
*guard = Some(ProcessedItems {
|
||||
items: matches,
|
||||
merge: merge_strategy,
|
||||
});
|
||||
}
|
||||
}
|
||||
});
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -18,10 +18,31 @@ use crate::{
|
|||
tui::widget::{SkimRender, SkimWidget},
|
||||
};
|
||||
|
||||
/// How to apply processed items to the display list
|
||||
#[derive(Default, Clone, Copy)]
|
||||
pub(crate) enum MergeStrategy {
|
||||
/// Replace the entire item list (full re-match or first result)
|
||||
#[default]
|
||||
Replace,
|
||||
/// Merge into existing list using sorted merge by rank
|
||||
SortedMerge,
|
||||
/// Append to existing list without sorting (for --no-sort)
|
||||
Append,
|
||||
}
|
||||
|
||||
/// Processed items ready for rendering
|
||||
#[derive(Default)]
|
||||
pub(crate) struct ProcessedItems {
|
||||
pub(crate) items: Vec<MatchedItem>,
|
||||
pub(crate) merge: MergeStrategy,
|
||||
}
|
||||
|
||||
impl Default for ProcessedItems {
|
||||
fn default() -> Self {
|
||||
Self {
|
||||
items: Vec::new(),
|
||||
merge: MergeStrategy::Replace,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Widget for displaying and managing the list of filtered items
|
||||
|
|
@ -571,7 +592,18 @@ impl SkimWidget for ItemList {
|
|||
);
|
||||
this.showing_stale_items = true;
|
||||
} else {
|
||||
this.items = processed.items;
|
||||
match processed.merge {
|
||||
MergeStrategy::Replace => {
|
||||
this.items = processed.items;
|
||||
}
|
||||
MergeStrategy::SortedMerge => {
|
||||
let existing = std::mem::take(&mut this.items);
|
||||
this.items = MatchedItem::sorted_merge(existing, processed.items);
|
||||
}
|
||||
MergeStrategy::Append => {
|
||||
this.items.extend(processed.items);
|
||||
}
|
||||
}
|
||||
this.showing_stale_items = false;
|
||||
|
||||
// Apply pre-selection only when new items arrive and only if we haven't reached target
|
||||
|
|
|
|||
Loading…
Reference in a new issue