Adopt and expose exponential backoff spinning #29

jonhoo commented 2016年06月02日 17:34:14 +02:00 (Migrated from github.com)

@Amanieu's parking_lot crate has a SpinWait module that implements exponential backoff. This would be a useful addition to the current implementation, and could also be exposed on its own. The latter might be useful when implementing other synchronization primitives that only need the spinning, and not the locking. There's been some discussion on this on the parking_lot issue tracker.

The biggest hurdle to adoption is that SpinWait currently yields after spinning for a while, which wouldn't work with no_std. As discussed on the parking_lot issue, this could be put behind a feature flag, or maybe even removed altogether and left to the users of the module.

@Amanieu's [parking_lot](https://github.com/Amanieu/parking_lot) crate has a [`SpinWait`](https://github.com/Amanieu/parking_lot/blob/master/src/spinwait.rs) module that implements exponential backoff. This would be a useful addition to the current implementation, and could also be exposed on its own. The latter might be useful when implementing other synchronization primitives that only need the spinning, and not the locking. There's been some discussion on this on the [parking_lot issue tracker](https://github.com/Amanieu/parking_lot/issues/5). The biggest hurdle to adoption is that `SpinWait` currently yields after spinning for a while, which wouldn't work with `no_std`. As discussed on the parking_lot issue, this could be put behind a feature flag, or maybe even removed altogether and left to the users of the module.

Ericson2314 commented 2016年06月02日 18:16:37 +02:00 (Migrated from github.com)

Ah I finally get that SpinWait is just for spinning + backoff. Yeah it can just be dropped in here, replacing https://github.com/mvdnes/spin-rs/blob/master/src/util.rs, with a feature for the std stuff.

For clarity, we can have methods spin_yield, spin_no_yield, and spin_default. The "std" feature, besides knocking out spin_yield, would affect which one spin_default calls. The locks themselves would call spin_default.

SpinWait, unlike util.rs should be exposed, because it is useful downstream too.

Ah I finally get that `SpinWait` is _just_ for spinning + backoff. Yeah it can just be dropped in here, replacing https://github.com/mvdnes/spin-rs/blob/master/src/util.rs, with a feature for the std stuff. For clarity, we can have methods `spin_yield`, `spin_no_yield`, and `spin_default`. The "std" feature, besides knocking out `spin_yield`, would affect which one `spin_default` calls. The locks themselves would call `spin_default`. `SpinWait`, unlike `util.rs` should be exposed, because it is useful downstream too.

Amanieu commented 2016年06月02日 18:35:19 +02:00 (Migrated from github.com)

I'd just like to put a disclaimer: the current implementation of SpinWait isn't very well tuned (read: I picked some numbers out of thin air and tweaked them until the results were good enough). In particular the optimal spin count can vary widely between different processors.

For reference, here is what Intel's TBB does: https://github.com/intel-tbb/intel-tbb/blob/master/include/tbb/tbb_machine.h#L349

I'd just like to put a disclaimer: the current implementation of `SpinWait` isn't very well tuned (read: I picked some numbers out of thin air and tweaked them until the results were good enough). In particular the optimal spin count can vary widely between different processors. For reference, here is what Intel's TBB does: https://github.com/intel-tbb/intel-tbb/blob/master/include/tbb/tbb_machine.h#L349

Ericson2314 commented 2016年06月02日 19:00:15 +02:00 (Migrated from github.com)

Sure, but that's still strictly better than what we have now :)

Sure, but that's still strictly better than what we have now :)

Ericson2314 commented 2016年06月02日 19:04:45 +02:00 (Migrated from github.com)

I'd just pull out 4 (for shifting) into a const for easy finding. Users that wish to tune for their own systems can use Cargo's [[replace]] to change the constant.

I'd just pull out 4 (for shifting) into a const for easy finding. Users that wish to tune for their own systems can use Cargo's `[[replace]]` to change the constant.

mvdnes commented 2016年06月04日 14:11:51 +02:00 (Migrated from github.com)

Seems like a good idea to handle spinning more efficiently.
If anyone submits a pull request I would be happy to take a look at it.

Seems like a good idea to handle spinning more efficiently. If anyone submits a pull request I would be happy to take a look at it.

arthurprs commented 2016年09月13日 15:02:49 +02:00 (Migrated from github.com)

parkinglot_core exposes it now, but the crate requires std and all. Maybe spin can copy that specific module sprinkling some conditional compilations around as suggested upthread.

parkinglot_core exposes it now, but the crate requires std and all. Maybe spin can copy that specific module sprinkling some conditional compilations around as suggested upthread.

zesterer commented 2020年10月10日 15:03:31 +02:00 (Migrated from github.com)

If I get the time, I plan to implement this next week.

If I get the time, I plan to implement this next week.