Skip to content

Ignore Miri leaks#1946

Closed
ghost wants to merge 1 commit intomasterfrom
unknown repository
Closed

Ignore Miri leaks#1946
ghost wants to merge 1 commit intomasterfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Dec 1, 2021

Issue

As discussed in #1927, Miri considers by default that a global ThreadPool is an error.

Polars ThreadPool

The following simple function gives an error according to Miri:

fn test_miri_rayonthread_leak1() {
    crate::POOL.install(|| ());
}

Rayon ThreadPool

The following simple function gives an error according to Miri:

fn test_miri_rayonthread_leak2() {
    use rayon::prelude::*;
    let tmp: Vec<i32> = [1, 2].par_iter().map(|x| x * x).collect();
    assert_eq!(tmp, &[1, 4]);
}

Solution

Adding -Zmiri-ignore-leaks to the MIRIFLAGS, as it seems to be the standard solution with Rayon.

What we loose

  • Miri doesn't check any memory or thread leak.

The serious memory leaks in Rust are not impossible but quite rare.

What we gain

- Performance

We can continue to use Rayon as much as possible without being annoyed by Miri. Indeed, many functions can still be improved in speed.

- Robustness

Making tests on functions using Rayon (namely thanks to our POOL ThreadPool) is possible in order to monitor the bugs.

Synthesis

In my opinion, the loss is tiny compared to the improvement on speed and robustness that can be done after this PR.

If we fear for memory leak in some cases, we can still use the default cargo miri manually. All the other Miri features are kept.

@ritchie46
Copy link
Member

I am not convinced we should do this.

For now the MIRI checks only the core part of the library, which IMO is sufficient. We can isolate single threaded tests and run those under MIRI.

The tests that run use the threadpool we can ignore under MIRI just like we did already.

@ghost
Copy link
Author

ghost commented Dec 1, 2021

Is there a trick to have a test that is checked by cargo test and not by Miri ?

@alippai
Copy link

alippai commented Dec 1, 2021

cfg flags

@ritchie46
Copy link
Member

Jeap..

    #[test]
    #[cfg_attr(miri, ignore)]
    fn test_foo() {
        let foo = std::str::from_utf8(&[102, 111, 111]).unwrap();
        assert_eq!("foo", foo);
    }

@ghost
Copy link
Author

ghost commented Dec 1, 2021

Ok, now I understand why the problem is in fact easy to solve.

Thank you for your patience.

@ghost ghost closed this Dec 1, 2021
This pull request was closed.
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.

2 participants