Switch back to std::sync::Mutex?

[SUPERCILEX]

rust-lang/rust#95035

std is using futex now and seems to beat parking lot in most cases. Are there contention benchmarks that could validate this for tokio?

[hawkw]

Tokio's use of parking_lot is an off-by-default optional dependency, so Tokio actually uses std::sync::Mutex by default, unless the parking_lot feature flag is enabled.

However, I agree that it would be interesting to measure performance with std's new mutex implementation; we might stop recommending the use of parking_lot if performance is consistently better with std now...

[SUPERCILEX]

Oops, I thought parking_lot was the default. Either way, if std faster I think parking_lot should just be removed entirely.

[Darksonn]

The parking_lot mutex has a const new method. We use that in some places, e.g. here.

[SUPERCILEX]

Dang, though that stuff should be able to be deprecated as std is moving towards removing the allocation that prevents things from being const: rust-lang/rust#93740

[hawkw]

Oops, I thought parking_lot was the default. Either way, if std faster I think parking_lot should just be removed entirely.

Removing the feature flag is a breaking change. It could be made into a no-op, where enabling it doesn't actually do anything and Tokio unconditionally would use the stdmutex, but this seems...questionable.

[SUPERCILEX]

How does deprecation work? Would the idea be to say the flag is deprecated (AFAIK there's no way to mark a flag as deprecated) and then remove it in a major release?

I think it's perfectly reasonable to make it a noop (I would even argue removal is fine without a major release) because parking_lot is specifically documented as an "internal feature." Unless there's a policy about internal features I'm missing?

[jdahlstrom]

Futexes are a Linux-only thing, remember.

[hawkw]

How does deprecation work? Would the idea be to say the flag is deprecated (AFAIK there's no way to mark a flag as deprecated) and then remove it in a major release?

There isn't any way to mark a feature flag itself as deprecated, but it could be made to emit a deprecation warning by enabling code that emits a deprecation warning. An otherwise unused type can be added and marked as deprecated to emit a deprecation warning when the feature is enabled.

Alternatively, a build script could check if the "deprecated" feature is enabled, and emit a warning using println!("cargo:warning=...);".

Both of these solutions are kind of hacky though.

[Noah-Kennedy]

Personally I see no reason to get rid of this feature.

[hawkw]

Yeah, IMO, the only compelling reason to remove the feature is to actually get rid of the need to test another feature combination, which we can't do without a breaking change anyway, so it's not going to happen any time soon. Users can always just choose not to enable parking_lot...which is what they get by default anyway.

[memoryruins]

The parking_lot mutex has a const new method. We use that in some places, e.g. here.

rust-lang/rust#97791 should make std's {Mutex, Condvar, RwLock}::new() const in rustc 1.63.0.

[Darksonn]

That's nice to know. Increasing our MSRV to 1.63.0 isn't going to happen anytime soon, but it probably will eventually.

[eaufavor]

Meanwhile, does it make sense to move parking_lot out of the full feature group? Other features don't change how tokio works the same way parking_lot does.

[Noah-Kennedy]

So for now that would be a breaking change due to const and MSRV. Once that change happens, I'd still say its a breaking change.

[nashley]

Increasing our MSRV to 1.63.0 isn't going to happen anytime soon, but it probably will eventually.

Once this happens, can tokio's Mutex::new() become const as well, or are there other reasons that preclude it from becoming const?

[hawkw]

Increasing our MSRV to 1.63.0 isn't going to happen anytime soon, but it probably will eventually.

Once this happens, can tokio's Mutex::new() become const as well, or are there other reasons that preclude it from becoming const?

Mutex::new could certainly become const fn once std::sync::Mutex::new is a const fn on Tokio's MSRV — note that when parking_lot is enabled, there's currently a separate const_new constructor. Once Tokio's MSRV is Rust 1.63.0 or newer, we could probably deprecate that in favor of making Mutex::new just be a const fn.

[NobodyXu]

Increasing our MSRV to 1.63.0 isn't going to happen anytime soon, but it probably will eventually.

Once this happens, can tokio's Mutex::new() become const as well, or are there other reasons that preclude it from becoming const?

Mutex::new could certainly become const fn once std::sync::Mutex::new is a const fn on Tokio's MSRV — note that when parking_lot is enabled, there's currently a separate const_new constructor. Once Tokio's MSRV is Rust 1.63.0 or newer, we could probably deprecate that in favor of making Mutex::new just be a const fn.

Unfortunately they can't since with cfg(all(tokio_unstable, feature = "tracing")) Mutex::new will log using tracing::trace! and it also constructs a span using tracing::trace_span!.