Miri is slow on what should be a straightforward program

[folkertdev]

This issue came up when chatting with @oli-obk. I fully understand that miri is not meant to be a fast interpreter, but small changes have delivered big performance wins in the past, so perhaps this example just hits some particular edge case that has a simple fix. The slow runtime (much worse on CI machines) limits how much we can use miri in practice.

git clone git@github.com:trifectatechfoundation/zlib-rs.git
cd zlib-rs
cargo +nightly miri nextest run --run-ignored=ignored-only deflate_init_uninitialized

the test in question only initializes the decompression state, and then disposes of it again. No data is actually compressed. The state is a bit over 3000 bytes on the stack. The initialization also allocates and zeroes a bunch of memory (on the order of 128kb). Otherwise it doesn't do anything particularly weird or computationally expensive:

#[test]
#[cfg_attr(miri, ignore = "slow")]
fn deflate_init_uninitialized() {
    use libz_rs_sys::*;

    unsafe {
        let mut strm = MaybeUninit::<z_stream>::uninit();

        core::ptr::write(
            core::ptr::addr_of_mut!((*strm.as_mut_ptr()).zalloc).cast(),
            zlib_rs::allocate::C.zalloc,
        );

        core::ptr::write(
            core::ptr::addr_of_mut!((*strm.as_mut_ptr()).zfree).cast(),
            zlib_rs::allocate::C.zfree,
        );

        core::ptr::write(
            core::ptr::addr_of_mut!((*strm.as_mut_ptr()).opaque),
            core::ptr::null_mut(),
        );

        let err = deflateInit_(
            strm.as_mut_ptr(),
            6,
            zlibVersion(),
            core::mem::size_of::<z_stream>() as _,
        );
        assert_eq!(err, Z_OK);

        let err = deflateEnd(strm.as_mut_ptr());
        assert_eq!(err, Z_OK);
    }
}

This test takes about 10s to run in isolation on my dev machine (running it alongside other tests slows it down further):

> cargo +nightly miri nextest run --run-ignored=ignored-only deflate_init_uninitialized
    Finished `test` profile [optimized + debuginfo] target(s) in 0.02s
info: for the target platform, using target runner `/home/folkert/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo-miri runner` defined by `target.cfg(all()).runner` specified by `--config`
info: for the host platform, using target runner `/home/folkert/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo-miri runner` defined by `target.cfg(all()).runner` specified by `--config`
    Starting 1 test across 4 binaries (291 skipped)
        PASS [  10.928s] test-libz-rs-sys null::deflate_init_uninitialized

Actually compressing the string "Ferris" adds 5 seconds to the runtime:

> cargo +nightly miri nextest run --run-ignored=ignored-only ferris_bytes
    Finished `test` profile [optimized + debuginfo] target(s) in 0.02s
info: for the target platform, using target runner `/home/folkert/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo-miri runner` defined by `target.cfg(all()).runner` specified by `--config`
info: for the host platform, using target runner `/home/folkert/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo-miri runner` defined by `target.cfg(all()).runner` specified by `--config`
    Starting 1 test across 4 binaries (291 skipped)
        PASS [  15.003s] test-libz-rs-sys null::ferris_bytes
------------
     Summary [  15.004s] 1 test run: 1 passed, 291 skipped

[saethlin]

Actually compressing the string "Ferris"

How would I do that?

[folkertdev]

it's the second example, so

cargo +nightly miri nextest run --run-ignored=ignored-only ferris_bytes

[saethlin]

If you add the flag -Zmiri-measureme, every interpreter will write a profdata file that you can consume with measureme's flamegraph tool.

This rapidly becomes a mess because actually running tests through Cargo involves spawning many interpreters in various directories. But for the ferris example, I've dug out the right profdata file and rendered the flamegraph:

rustc.svg.gz

...and it's basically all in slice filling. Ouch. Maybe you can do something with that? I suspect that calling into write_bytes would be faster to execute in the interpreter. And perhaps that is a useful thing for slice::fill in general even? 🤷

But being able to render these flamegraphs should empower you to experiment on strategies to improve the runtime of your test suite, and maybe you'll come up with something more clever.

[folkertdev]

I'm assuming it's this

https://github.com/trifectatechfoundation/zlib-rs/blob/67beda38afd6b658031e3c8ef1574bdab0c5b508/zlib-rs/src/deflate.rs#L331-L339

The type of head is *mut [u16; 65536], so that's big. prev is same order of magnitude. Using calloc instead does not really appear to help total runtime.

Can't miri optimize memset better?

[the8472]

...and it's basically all in slice filling. Ouch. Maybe you can do something with that? I suspect that calling into write_bytes would be faster to execute in the interpreter. And perhaps that is a useful thing for slice::fill in general even? 🤷

We had that (#83245), but then it had to be removed due to unsoundness rust-lang/rust#87891 rust-lang/rust#87892.

To add it back the write_bytes intrinsic needs to be generalized to take MaybeUninit<u8> instead of u8.

[saethlin]

Can't miri optimize memset better?

If you actually call memset from Miri, the implementation should be reasonably fast and it should be quite clear that it is actually a call to memset based on the measureme flamegraph.

But if you call into something that is conceptually equivalent to memset, then it will be slow.

[folkertdev]

ok lol, this patch gives like a 10x speedup.

https://github.com/trifectatechfoundation/zlib-rs/compare/unsafe-fill

I'd really rather not resort to unsafe just for miri. Looking into the8472 's comment, I found

/// This is an accidentally-stable alias to [`ptr::write_bytes`]; use that instead.
// Note (intentionally not in the doc comment): `ptr::write_bytes` adds some extra
// debug assertions; if you are writing compiler tests or code inside the standard library
// that wants to avoid those debug assertions, directly call this intrinsic instead.
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_allowed_through_unstable_modules = "import this function via `std::ptr` instead"]
#[rustc_const_stable(feature = "const_intrinsic_copy", since = "1.83.0")]
#[rustc_nounwind]
#[rustc_intrinsic]
pub const unsafe fn write_bytes<T>(dst: *mut T, val: u8, count: usize);

Normally it should be fine to change that to MaybeUninit I think, but this intrinsic is apparently stable by accident?! What does that mean, can it be changed, or is a new intrinsic needed?

[the8472]

You can have separate code-paths for miri via cfg(miri).

[folkertdev]

Sure, yeah we can solve this, and I just realized that the u16 fill would not even be covered by that old specialization. However, as a user, I totally expect .fill(0) to just be a memset, for any numeric type honestly. This is a footgun that, if possible, I'd like to fix.