-
Notifications
You must be signed in to change notification settings - Fork 412
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
give tantivy a shared thread pool to run fetch doc on #4942
Conversation
.build() | ||
.expect("Failed to spawn the spawning pool") | ||
}) | ||
let executor_ptr = Arc::as_ptr(SEARCH_THREAD_POOL.get_or_init(build_executor)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is correct, but could break if we modified a different part of the code.
It is sometimes acceptable (especially because it is in the same file, and private stuff), but still, I suspect this is unnecessary here.
We could put in the Arc on the stack of the run_cpu_intensive function and extract the ref here (without a &'static lifetime).
Even better I think we can call spawn_blocking directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub async fn run_cpu_intensive<F, R>(cpu_heavy_task: F) -> Result<R, Panicked>
where
F: FnOnce() -> R + Send + 'static,
R: Send + 'static,
{
let span = tracing::Span::current();
search_executor().spawn_blocking(move || {
let _guard = span.enter();
let mut active_thread_guard =
GaugeGuard::from_gauge(&crate::SEARCH_METRICS.active_search_threads_count);
active_thread_guard.add(1i64);
cpu_heavy_task()
}).await.map_err(|_| Panicked)
}
(marking as wip until it depends on tantivy/main and not an unmerged branch) |
5420d5c
to
828535b
Compare
828535b
to
221b673
Compare
Description
likely to fix #4940
useless without upgrading tantivy post quickwit-oss/tantivy#2386