perf(daemon): answer search polls with counts instead of copies
A CLI one-off search leaves its result set in the daemon until someone forgets it, and an attached TUI made that permanent state expensive: every 100ms tick asked for the search list twice, and each answer deep-cloned every result of every search under the context lock just to sum file counts - stalling the thread that stores incoming results. Worse, any active or selected search pulled its entire result set over the socket again each tick, megabytes of JSON repeating what the window already held. search.list is now backed by search_file_counts, a walk of the cache instead of a copy of it; the TUI syncs once per tick and re-fetches a result set only when the session's file count differs from what it holds. get_search_results_count reads the same summary, so get --pick first against a daemon polls a count instead of the full set, and both session kinds now count files where the local one counted peer responses.
This commit is contained in:
+15
-4
@@ -86,6 +86,8 @@ pub trait SessionApi: Send + Sync {
|
||||
/// Drop a search from the session, so every window loses it rather than
|
||||
/// only the one that dismissed it.
|
||||
fn forget_search(&self, query: &str) -> bool;
|
||||
/// How many files a search has collected so far — a count cheap enough
|
||||
/// to poll, never a copy of the results.
|
||||
fn get_search_results_count(&self, key: &str) -> usize;
|
||||
fn try_get_search_results(&self, key: &str) -> Option<Vec<SearchResult>>;
|
||||
fn start_wishlist_search(&self, query: &str) -> Result<()>;
|
||||
@@ -198,11 +200,15 @@ impl SessionApi for Client {
|
||||
}
|
||||
|
||||
fn all_searches(&self) -> Vec<SessionSearch> {
|
||||
Self::get_all_searches(self)
|
||||
// Counts, not copies: the daemon answers this for every attached
|
||||
// client many times a second, and cloning every result set to sum
|
||||
// its lengths held the context lock against the thread storing new
|
||||
// results.
|
||||
Self::search_file_counts(self)
|
||||
.into_iter()
|
||||
.map(|(query, search)| SessionSearch {
|
||||
.map(|(query, files)| SessionSearch {
|
||||
query,
|
||||
files: search.results.iter().map(|r| r.files.len()).sum(),
|
||||
files,
|
||||
// A local window started every search it can see, so it
|
||||
// already knows which are still running.
|
||||
started_secs_ago: None,
|
||||
@@ -211,7 +217,12 @@ impl SessionApi for Client {
|
||||
}
|
||||
|
||||
fn get_search_results_count(&self, key: &str) -> usize {
|
||||
Self::get_search_results_count(self, key)
|
||||
// Files, agreeing with the remote session — the library's own
|
||||
// accessor of this name counts per-peer responses instead.
|
||||
Self::search_file_counts(self)
|
||||
.into_iter()
|
||||
.find(|(query, _)| query == key)
|
||||
.map_or(0, |(_, files)| files)
|
||||
}
|
||||
|
||||
fn try_get_search_results(&self, key: &str) -> Option<Vec<SearchResult>> {
|
||||
|
||||
@@ -544,7 +544,12 @@ impl SessionApi for RemoteSession {
|
||||
}
|
||||
|
||||
fn get_search_results_count(&self, key: &str) -> usize {
|
||||
self.get_search_results(key).len()
|
||||
// The summary's file count, not the full set: callers poll this to
|
||||
// ask whether anything has arrived, and the set can be megabytes.
|
||||
self.all_searches()
|
||||
.into_iter()
|
||||
.find(|search| search.query == key)
|
||||
.map_or(0, |search| search.files)
|
||||
}
|
||||
|
||||
fn try_get_search_results(&self, key: &str) -> Option<Vec<SearchResult>> {
|
||||
|
||||
@@ -274,6 +274,14 @@ mod tests {
|
||||
history: Vec<ChatMessageDto>,
|
||||
downloads: Vec<soulseek_rs::types::Download>,
|
||||
searches: std::sync::Mutex<Vec<crate::api::SessionSearch>>,
|
||||
/// What `try_get_search_results` hands back to the window.
|
||||
results: std::sync::Mutex<Vec<soulseek_rs::SearchResult>>,
|
||||
/// How many times a window asked for the search list.
|
||||
all_searches_calls: std::sync::atomic::AtomicUsize,
|
||||
/// How many times a window pulled a full result set across. Against a
|
||||
/// daemon each pull is the whole set over the socket, so the tests
|
||||
/// count them.
|
||||
result_fetches: std::sync::atomic::AtomicUsize,
|
||||
/// Whether this session is a daemon's. Local sessions do not sync.
|
||||
shared: bool,
|
||||
/// What `set_download_directory` was last asked for, if anything.
|
||||
@@ -328,6 +336,8 @@ mod tests {
|
||||
Vec::new()
|
||||
}
|
||||
fn all_searches(&self) -> Vec<crate::api::SessionSearch> {
|
||||
self.all_searches_calls
|
||||
.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
|
||||
self.searches.lock().expect("not poisoned").clone()
|
||||
}
|
||||
fn forget_search(&self, query: &str) -> bool {
|
||||
@@ -343,7 +353,9 @@ mod tests {
|
||||
&self,
|
||||
_key: &str,
|
||||
) -> Option<Vec<soulseek_rs::SearchResult>> {
|
||||
None
|
||||
self.result_fetches
|
||||
.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
|
||||
Some(self.results.lock().expect("not poisoned").clone())
|
||||
}
|
||||
fn start_wishlist_search(
|
||||
&self,
|
||||
@@ -512,8 +524,12 @@ mod tests {
|
||||
}
|
||||
|
||||
fn with_session(session: TalkativeSession) -> MainTui {
|
||||
attach(Arc::new(session))
|
||||
}
|
||||
|
||||
fn attach(session: Arc<TalkativeSession>) -> MainTui {
|
||||
MainTui::new(
|
||||
Arc::new(session),
|
||||
session,
|
||||
"/tmp".to_string(),
|
||||
Duration::from_secs(1),
|
||||
None,
|
||||
@@ -689,6 +705,130 @@ mod tests {
|
||||
assert!(tui.state.searches.is_empty());
|
||||
}
|
||||
|
||||
fn files_from_bob(names: &[&str]) -> soulseek_rs::SearchResult {
|
||||
soulseek_rs::SearchResult {
|
||||
token: 0,
|
||||
files: names
|
||||
.iter()
|
||||
.map(|name| soulseek_rs::types::File {
|
||||
username: "bob".to_string(),
|
||||
name: (*name).to_string(),
|
||||
size: 42,
|
||||
attribs: std::collections::HashMap::new(),
|
||||
})
|
||||
.collect(),
|
||||
slots: 1,
|
||||
speed: 0,
|
||||
username: "bob".to_string(),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_tick_asks_the_session_for_its_searches_once() {
|
||||
// Against a daemon every call is a round trip, and the daemon answers
|
||||
// it by walking its whole search cache — twice per frame was half the
|
||||
// cost of attaching at all.
|
||||
let session = Arc::new(shared(vec![search_of("q", 0, 9_999)]));
|
||||
let mut tui = attach(session.clone());
|
||||
|
||||
tui.update_search_results();
|
||||
|
||||
assert_eq!(
|
||||
session
|
||||
.all_searches_calls
|
||||
.load(std::sync::atomic::Ordering::Relaxed),
|
||||
1,
|
||||
"one sync means one listing, not one per use of it"
|
||||
);
|
||||
}
|
||||
|
||||
/// An attached window that selected a two-file search and has already
|
||||
/// ticked past its one necessary fetch.
|
||||
fn window_that_pulled_two_files() -> (Arc<TalkativeSession>, MainTui) {
|
||||
let session = Arc::new(TalkativeSession {
|
||||
results: std::sync::Mutex::new(vec![files_from_bob(&[
|
||||
"@@x\\a.mp3",
|
||||
"@@x\\b.mp3",
|
||||
])]),
|
||||
..shared(vec![search_of("aphex twin", 2, 9_999)])
|
||||
});
|
||||
let mut tui = attach(session.clone());
|
||||
tui.state.selected_search_index = Some(0);
|
||||
tui.update_search_results();
|
||||
tui.update_search_results();
|
||||
(session, tui)
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_selected_search_with_nothing_new_is_not_pulled_across_again() {
|
||||
// The result set only accumulates, so an unchanged count means a
|
||||
// fetch would hand back what this window already holds — against a
|
||||
// daemon that is the entire result set over the socket, every frame,
|
||||
// forever.
|
||||
let (session, tui) = window_that_pulled_two_files();
|
||||
|
||||
assert_eq!(tui.state.searches[0].results.len(), 2);
|
||||
assert_eq!(
|
||||
session
|
||||
.result_fetches
|
||||
.load(std::sync::atomic::Ordering::Relaxed),
|
||||
1,
|
||||
"the set crosses once; after that the unchanged count spares it"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn fresh_results_still_reach_a_window_that_skipped_a_fetch() {
|
||||
let (session, mut tui) = window_that_pulled_two_files();
|
||||
|
||||
// A late responder lands in the session.
|
||||
*session.searches.lock().expect("not poisoned") =
|
||||
vec![search_of("aphex twin", 3, 9_999)];
|
||||
*session.results.lock().expect("not poisoned") =
|
||||
vec![files_from_bob(&["@@x\\a.mp3", "@@x\\b.mp3", "@@x\\c.mp3"])];
|
||||
tui.update_search_results();
|
||||
|
||||
assert_eq!(
|
||||
tui.state.searches[0].results.len(),
|
||||
3,
|
||||
"a changed count must bring the new results across"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_local_window_fetches_results_without_counting_on_the_session() {
|
||||
// Locally nothing maintains known_files, so the count gate must stay
|
||||
// out of the way: a search whose count still reads zero has to get
|
||||
// its results anyway.
|
||||
let session = Arc::new(TalkativeSession {
|
||||
results: std::sync::Mutex::new(vec![files_from_bob(&[
|
||||
"@@x\\a.mp3",
|
||||
"@@x\\b.mp3",
|
||||
])]),
|
||||
shared: false,
|
||||
..TalkativeSession::default()
|
||||
});
|
||||
let mut tui = attach(session);
|
||||
tui.state.searches.push(crate::models::SearchEntry {
|
||||
query: "mine".to_string(),
|
||||
status: SearchStatus::Active,
|
||||
results: Vec::new(),
|
||||
known_files: 0,
|
||||
owned: true,
|
||||
start_time: std::time::Instant::now(),
|
||||
cancel_flag: Arc::new(std::sync::atomic::AtomicBool::new(false)),
|
||||
});
|
||||
|
||||
tui.update_search_results();
|
||||
|
||||
assert_eq!(
|
||||
tui.state.searches[0].results.len(),
|
||||
2,
|
||||
"a zero count matching zero held rows must not read as \
|
||||
nothing-to-fetch on a local session"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_window_with_its_own_session_is_left_alone() {
|
||||
// Nothing to share, so nothing is synced — and in particular the
|
||||
|
||||
@@ -231,23 +231,23 @@ impl MainTui {
|
||||
}
|
||||
|
||||
/// Show the searches the session knows about, not just this window's.
|
||||
/// Returns whether it synced, which is only the case against a daemon.
|
||||
///
|
||||
/// Against a daemon the search cache is shared, so a window that opens
|
||||
/// later sees what the others have looked for, and a query run in one
|
||||
/// appears in the other. A search is still collecting if it went out less
|
||||
/// than one search window ago — the window belongs to whichever client
|
||||
/// started it, so the age is the only thing another window can judge by.
|
||||
fn sync_searches_with_session(&mut self) {
|
||||
fn sync_searches_with_session(&mut self) -> bool {
|
||||
// Only worth doing when the session is shared: a window with its own
|
||||
// session ran every search in the list and already knows their state,
|
||||
// so asking would cost a full copy of every result set per frame and
|
||||
// tell us nothing.
|
||||
// session ran every search in the list and already knows their state.
|
||||
if self.client.daemon_endpoint().is_none() {
|
||||
return;
|
||||
return false;
|
||||
}
|
||||
|
||||
let timeout = self.search_timeout.as_secs();
|
||||
for known in self.client.all_searches() {
|
||||
let known_searches = self.client.all_searches();
|
||||
for known in &known_searches {
|
||||
let running = known
|
||||
.started_secs_ago
|
||||
.is_some_and(|elapsed| elapsed < timeout);
|
||||
@@ -267,13 +267,15 @@ impl MainTui {
|
||||
} else {
|
||||
SearchStatus::Completed
|
||||
};
|
||||
existing.known_files = known.files;
|
||||
}
|
||||
// The count is every entry's change detector: as long as it
|
||||
// matches what this window holds, there is nothing to fetch.
|
||||
existing.known_files = known.files;
|
||||
continue;
|
||||
}
|
||||
|
||||
self.state.searches.push(SearchEntry {
|
||||
query: known.query,
|
||||
query: known.query.clone(),
|
||||
status: if running {
|
||||
SearchStatus::Active
|
||||
} else {
|
||||
@@ -291,36 +293,41 @@ impl MainTui {
|
||||
|
||||
// Dismissed elsewhere means dismissed here. Ours stay: this window is
|
||||
// the authority on a search it started.
|
||||
let live: Vec<String> = self
|
||||
.client
|
||||
.all_searches()
|
||||
.into_iter()
|
||||
.map(|search| search.query)
|
||||
.collect();
|
||||
self.state
|
||||
.searches
|
||||
.retain(|search| search.owned || live.contains(&search.query));
|
||||
self.state.searches.retain(|search| {
|
||||
search.owned
|
||||
|| known_searches
|
||||
.iter()
|
||||
.any(|known| known.query == search.query)
|
||||
});
|
||||
true
|
||||
}
|
||||
|
||||
pub(super) fn update_search_results(&mut self) {
|
||||
self.sync_searches_with_session();
|
||||
// Only a synced session reports counts, so only there can an
|
||||
// unchanged count spare the fetch. Locally `known_files` is never
|
||||
// maintained and skipping on it would starve every search.
|
||||
let counted = self.sync_searches_with_session();
|
||||
let timeout = self.search_timeout;
|
||||
let selected_search_index = self.state.selected_search_index;
|
||||
|
||||
// Fetch all results in one go (single lock acquisition per query)
|
||||
// Use try_get_search_results to avoid blocking the UI thread
|
||||
// Only what the user can see or what is still arriving. A shared
|
||||
// cache can hold every search every window has ever run, and fetching
|
||||
// all of them each frame is a round trip apiece against a daemon.
|
||||
// The rest show the count the session reported.
|
||||
// Only what the user can see or what is still arriving, and against
|
||||
// a daemon only when the session's count says something actually
|
||||
// changed: results accumulate, so an unchanged count means a fetch
|
||||
// would carry the entire set across the socket to learn nothing.
|
||||
// A re-searched query resets the session's set; only a refill that
|
||||
// lands back on exactly the held total within one tick could leave
|
||||
// stale rows, and that window was already showing them.
|
||||
let all_results: Vec<(usize, Vec<_>)> = self
|
||||
.state
|
||||
.searches
|
||||
.iter()
|
||||
.enumerate()
|
||||
.filter(|(idx, search)| {
|
||||
search.status == SearchStatus::Active
|
||||
|| selected_search_index == Some(*idx)
|
||||
(search.status == SearchStatus::Active
|
||||
|| selected_search_index == Some(*idx))
|
||||
&& (!counted || search.known_files != search.results.len())
|
||||
})
|
||||
.map(|(idx, s)| (idx, s.query.clone()))
|
||||
.filter_map(|(idx, query)| {
|
||||
@@ -370,13 +377,16 @@ impl MainTui {
|
||||
self.state.results_filtered_indices = indices;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Mark as completed after timeout
|
||||
if search.status == SearchStatus::Active
|
||||
&& search.start_time.elapsed() > timeout
|
||||
{
|
||||
search.status = SearchStatus::Completed;
|
||||
}
|
||||
// Outside the fetch loop: a tick that had nothing to fetch must
|
||||
// still finish a run-out search.
|
||||
for search in &mut self.state.searches {
|
||||
if search.status == SearchStatus::Active
|
||||
&& search.start_time.elapsed() > timeout
|
||||
{
|
||||
search.status = SearchStatus::Completed;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user