Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adopt and expose exponential backoff spinning #29

Open
jonhoo opened this issue Jun 2, 2016 · 7 comments
Open

Adopt and expose exponential backoff spinning #29

jonhoo opened this issue Jun 2, 2016 · 7 comments

Comments

@jonhoo
Copy link

jonhoo commented Jun 2, 2016

@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.

@Ericson2314
Copy link
Contributor

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
Copy link

Amanieu commented Jun 2, 2016

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
Copy link
Contributor

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

@Ericson2314
Copy link
Contributor

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
Copy link
Owner

mvdnes commented Jun 4, 2016

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
Copy link
Contributor

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
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants