feat: use a separate thread pool for Matcher (and perhaps for all of skim)

[kimono-koans]

Description of the changes

Use separate thread pool for Matcher. Using the rayon global thread may interfere with apps running in that thread pool and/or effect interactivity.

Maybe able to de-initialize the static MATCHER_THREAD_POOL by using OnceLock, instead of LazyLock, and using take() to return it to a de-initialized state. See: https://doc.rust-lang.org/std/sync/struct.OnceLock.html#method.take

[LoricAndre]

This looks really interesting, and I agree it is needed. The only doubt I have is whether we should make it opt-in or opt-out, I think opt-in makes more sense since the wide majority of our users are binary users, not library ones. I'm not an expert on rayon, but maybe we could add the rayon pool as a field of Matcher, and be able to either manually create one and set it or have a separate_thread_pool method that sets it up for you as you did. We would need to make sure we don't recreate the thread pool every time, otherwise that would be catastrophic.

[kimono-koans]

The only doubt I have is whether we should make it opt-in or opt-out, I think opt-in makes more sense since the wide majority of our users are binary users, not library ones.

Running with a separate thread pool should be better for CLI reactivity too?

We would need to make sure we don't recreate the thread pool every time, otherwise that would be catastrophic.

Pretty certain setting as static prevents this, but one could just as easily not create the pool in a LazyLock (which is a lazy way to do this with more thread bureaucracy), and create at Skim invocation, and pass as a variable to each new MatcherControl, like so:

pub struct App<'a> {
    pub thread_pool: Arc<ThreadPool>,
...
}

[LoricAndre]

I prefer having it at app level, as this allows us to still use the global pool if needed, thanks

[LoricAndre]

The tests are failing because of flakyness in the status display induced by these changes, could you take a look and fix them ? I'm seeing a lot of flaky tests failing up to 5 times, but I generally don't have flakies locally, sometimes just one or 2 failing once, very rarely more.

[kimono-koans]

The tests are failing because of flakyness in the status display induced by these changes, could you take a look and fix them ? I'm seeing a lot of flaky tests failing up to 5 times, but I generally don't have flakies locally, sometimes just one or 2 failing once, very rarely more.

Are you sure these test aren't broken?

➜  two_percent git:(master) cargo test
   Compiling skim v3.0.1 (/srv/program/two_percent)
error[E0599]: no function or associated item named `new_for_test` found for struct `Tui<_>` in the current scope
   --> tests/common/insta.rs:401:20
    |
401 |     let tui = Tui::new_for_test(backend)?;
    |                    ^^^^^^^^^^^^ function or associated item not found in `Tui<_>`
    |
note: if you're trying to build a new `Tui<_>` consider using one of the following associated functions:
      Tui::new_with_height
      Tui::<B>::new_with_height_and_backend
   --> src/tui/backend.rs:51:5
    |
 51 |     pub fn new_with_height(height: Size) -> Result<Self> {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
 62 |     pub fn new_with_height_and_backend(backend: B, height: Size) -> Result<Self> {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

For more information about this error, try `rustc --explain E0599`.
error: could not compile `skim` (test "case") due to 1 previous error
warning: build failed, waiting for other jobs to finish...
error: could not compile `skim` (test "preview") due to 1 previous error

[LoricAndre]

You need to run them using cargo insta + cargo nextest and the test-utils feature, it should be explained in CONTRIBUTING.md

[LoricAndre]

I guess it may have been my fault when I rebased, sorry about that

[LoricAndre]

I guess it may have been my fault when I rebased, sorry about that

[kimono-koans]

I guess it may have been my fault when I rebased, sorry about that

I'm just not sure about why/how these tests are failing. The tests are not documented and there is no assertion of what the failure is.

Near as I can tell -- the snapshots do not match?

Now, my guess is the snapshots are reading at some interstitial/inflight moment. But imagine you want to write to an AtomicUsize once when processing a 4096 chunk, instead of upon each single update. Is there some semantic reason that's wrong? Not as far as I can tell?

[LoricAndre]

I'll try to take a look at the test harness itself, but it'll have to wait. I agree that the tests are a bit opaque, especially when the failures are due to things like the spinner still showing up
Unfortunately, TUIs are hard to test by nature, and skim definitely is no exception. We're closing in on 400 tests now and we still see regression regularly 😅
Thank you for still taking the time to look into this

[codecov]

Codecov Report

❌ Patch coverage is 85.00000% with 15 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/lib.rs	86.53%	6 Missing and 1 partial ⚠️
src/tui/app.rs	70.00%	6 Missing ⚠️
src/matcher.rs	92.85%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[LoricAndre]

Thank you, the flakies were due to rayon thread allocation subtleties and a small timing issue
I merged it and I plan on releasing a pretty huge perf and memory improvement in a few days, making us measurably faster than fzf with similar memory and better CPU usage