tokio-rs / loom

Concurrency permutation testing tool for Rust.
MIT License
2.15k stars 112 forks source link

Is it possible for `loom::sync::Mutex::new` to be const available? #320

Closed NobodyXu closed 1 year ago

NobodyXu commented 1 year ago

This comes from https://github.com/tokio-rs/tokio/pull/5885#issuecomment-1653701758 :

I think it's better to do this in a separate PR and it will require quite some duplication since rust doesn't permit cfg_attr(not(all(loom, test)), const).

If loom::sync::Mutex::new is available in const then tokio can remove the bound cfg(not(all(loom, test))) and make all {Mutex, Notify, ..}::new available in const without having to define two different versions depending on cfg(not(all(loom, test))).

From loom::sync::Mutex source code:

#[derive(Debug)]
pub struct Mutex<T> {
    object: rt::Mutex,
    data: std::sync::Mutex<T>,
}

it seems that it's mainly rt::Mutex and the MSRV blocking the method from being const.

rt::Mutex relies on global STATE, so the only way to make it const is to use OnceCell to initialize it lazily.

Darksonn commented 1 year ago

I don't think that would be possible. Loom primitives must be dropped and recreated between each run.

NobodyXu commented 1 year ago

I don't think that would be possible. Loom primitives must be dropped and recreated between each run.

They will still get dropped and recreated during each run.

What I meant is to change rt::Mutex to this:

#[derive(Debug, Copy, Clone)]
pub(crate) struct Mutex {
    state: OnceCell<object::Ref<State>>,
}
Darksonn commented 1 year ago

If its in a global, then you can't recreate it after it is first initialized?

NobodyXu commented 1 year ago

If its in a global, then you can't recreate it after it is first initialized?

No it's not global, it's still a local variable, just that it uses OnceCell to initialize lazily as opposed to initialize eagerly.

That will allow rt::Mutex::new to be const.

NobodyXu commented 1 year ago

If you are ok with bumping msrv to 1.63, adding once_cell as a new dependency, I can submit two PRs for this (one for MSRV and other one for making it const).

Darksonn commented 1 year ago

I don't think its necessary for us to do this. Even if we don't use it in globals, making it const will make people think that using it in globals is possible, but it is incorrect and will not work.

We can duplicate the new methods instead.

NobodyXu commented 1 year ago

I don't think its necessary for us to do this. Even if we don't use it in globals, making it const will make people think that using it in globals is possible, but it is incorrect and will not work.

We can duplicate the new methods instead.

Yeah I agree that duplicating new and have a const_new is better than marking new as const, although it will still have to use OnceCell to delay initialization to runtime instead of at compile-time.