Skip to content
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

Merged
merged 4 commits into from
May 9, 2024

Conversation

trinity-1686a
Copy link
Contributor

Description

likely to fix #4940
useless without upgrading tantivy post quickwit-oss/tantivy#2386

@trinity-1686a trinity-1686a requested a review from fulmicoton May 6, 2024 15:39
.build()
.expect("Failed to spawn the spawning pool")
})
let executor_ptr = Arc::as_ptr(SEARCH_THREAD_POOL.get_or_init(build_executor));
Copy link
Contributor

@fulmicoton fulmicoton May 7, 2024

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.

Copy link
Contributor

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)
}

@trinity-1686a trinity-1686a marked this pull request as draft May 7, 2024 12:33
@trinity-1686a
Copy link
Contributor Author

(marking as wip until it depends on tantivy/main and not an unmerged branch)

@fulmicoton fulmicoton marked this pull request as ready for review May 9, 2024 10:20
@fulmicoton fulmicoton force-pushed the trinity/shared-thread-pool-tantivy branch from 5420d5c to 828535b Compare May 9, 2024 10:24
@fulmicoton fulmicoton force-pushed the trinity/shared-thread-pool-tantivy branch from 828535b to 221b673 Compare May 9, 2024 10:27
@fulmicoton fulmicoton enabled auto-merge (squash) May 9, 2024 11:08
@fulmicoton fulmicoton merged commit 4a327a1 into main May 9, 2024
5 checks passed
@fulmicoton fulmicoton deleted the trinity/shared-thread-pool-tantivy branch May 9, 2024 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Searcher gets deconnected on query
2 participants