Consider switching off spin

[Evrey]

So, apparently spin is unmaintained. cargo deny mentions a few alternatives, though.

error[RUSTSEC-2019-0031]: spin is no longer actively maintained
    ┌─ /×××××××/Cargo.lock:103:1
    │
103 │ spin 0.5.2 registry+https://github.com/rust-lang/crates.io-index
    │ ---------------------------------------------------------------- unmaintained advisory detected
    │
    = The author of the `spin` crate does not have time or interest to maintain it.
      
      Consider the following alternatives (both of which support `no_std`):
      
      - [`conquer-once`](https://github.com/oliver-giersch/conquer-once)
      - [`lock_api`](https://crates.io/crates/lock_api) (a subproject of `parking_lot`)
        - [`spinning_top`](https://github.com/rust-osdev/spinning_top) spinlock crate built on `lock_api`
    = URL: https://github.com/mvdnes/spin-rs/commit/7516c80
    = spin v0.5.2
      └── flume v0.7.1
          └── ×××××××

2020-05-29 16:19:01 [ERROR] encountered 1 errors

[matklad]

Note also that spinlocks have pretty bad worst-case behavior. If performance of std::Mutex is unsatisfactory, i would advice to replace it with a faster sleeping mutex (eg, parking_lot) instead of going to an unbounded spinning:

https://matklad.github.io/2020/01/02/spinlocks-considered-harmful.html

[zesterer]

@matklad Hi. I read your blog post some months ago. While it makes a lot of good points, I don't think it's applicable in this case. Flume doesn't use spinlocks like a regular mutex: it uses them to moderate contention and then switches to scheduler-managed waiting after a backoff period. We've done extensive benchmarking across a wide variety of scenarios and have found this to consistently perform better than a mutex under many different workloads on *nix platforms.

[Restioson]

With regards to the process we went through on this:

• We tested spinlock vs std mutex on Linux. Spinlocks were better.
• Then, we tested spinlocks vs std mutex on Windows. Mutexes were a lot better, so we added a cfg to use them on Windows.
• We looked at parking_lot but at the time thought that it would be too heavy a dependency and could not survive a potential integration of flume into the stdlib (as parking_lot depends on std). Performance was better with parking_lot, iirc.

Imo, still worth looking into potentially replacing spin considering it is unmaintained and therefore triggers cargo deny. But prob keeping the spinlock from our testing, unless something changes.

[matklad]

Flume doesn't use spinlocks like a regular mutex: it uses them to moderate contention and then switches to scheduler-managed waiting after a backoff period.

Is it? Perhaps I am reading the code incorrectly, but it seems there are cases where flume doesn't fall-back to scheduler-managed waiting?

Here's what I gathered (might be totally or partially wrong):

• flume imports spin::Mutex
• spin::Mutex does not fall back to making syscalls to notify the scheduler about waiting
• flume locks spin::Mutex without any kind of timeout

So, either I missed the bit about falling back to scheduler, or it isn't there?

[matklad]

We looked at parking_lot but at the time thought that it would be too heavy a dependency and could not survive a potential integration of flume into the stdlib

JFYI, the way I deal with it in once_cell is that parking_lot is an optional non-default feature, and the internal impl switches between std-based impl and a parking-lot based one. Seems like this might be a nice option for flume as well?

[zesterer]

@matklad lock_inner is used on Windows, and it uses std::sync::Mutex (see the import at the top of the file). For *nix systems, that use spinning, this is the code you want to be looking at:

flume/src/lib.rs

Line 306 in 53d93a0

fn wait_inner(&self) -> MutexGuard<'_, Inner<T>> {

[matklad]

Ah, indeed. Though I wonder why lock_inner itself is not guarded with #[cfg(not(windows))]? it seems like sometimes it is used on nixes as well?

flume/src/lib.rs

Lines 591 to 595 in 53d93a0

	#[cfg(feature = "select")]
	#[inline]
	fn connect_recv_selector(&self, signal: Arc<Signal<Token>>, token: Token) {
	self.lock_inner().recv_selector = Some((signal, token));
	}

[zesterer]

@matklad Yes, it's used during shutdown. Most work is now being done on the flavour-refactor branch. master, while functional, is a bit untidy.

[zesterer]

spin has now been replaced with spinning_top by #24