Rewrite RwLock to use folly upgradeable implementation

[64]

Fixes #65.

I fully expect there to be some issues with this, so I'm going to get a few more eyes to look over it.

I've done a very quick performance test with the parking lot benchmarks:

$ cargo run --bin rwlock --release 1 3 100 10000 10 5
spin::RwLock (new)   - [write]     43.180 kHz [read]    129.347 kHz
spin::RwLock (old)   - [write]     43.146 kHz [read]    129.297 kHz

I genuinely have no idea whether those arguments make for a good test lol. Happy to rerun with something else.

Also not sure how you want to handle semver here.

[64]

I think I can improve the API with a bit of unsafe magic. Specifically I’d like to move the upgrade functions onto the RAII guard itself. Will try tomorrow

[Ericson2314]

nit: I would use 1 << 2 here for clarity.

[Ericson2314]

Yeah this looks pretty good overall, thanks for doing this!

[Ericson2314]

Can we use a higher order function to abstract over compare_exchange_weak vs compare_exchange? #[inline_always] it just to make sure it's compiled away.

[64]

hm, you mean like some free function fn compare_exchange(current: usize, new: usize, success: Ordering, failure: Ordering, strong: bool)?

[Ericson2314]

I was thinking like

fn make_try_write(
    &self,
    comparer: fn(&AtomicUSize, isize, isize, Ordering, failure: Ordering) -> Result<isize, isize>
) -> ...

but yeah yours works too.

[Ericson2314]

I feel like it might be possible to optimize this? What about:

let old = self.lock.load(Ordering::Relaxed);
self.lock.store((old & !UPGRADED) + READER, Ordering::Release);
let new_guard = ...;
mem::forget(self);
new_guard

and same for write lock downgrade. Since the readers cannot change while a write or upgradable lock is held, there is no race condition between the load and store, and since the downgrade is conceptually a partial release of the lock there is no unnecessary acquiring.

[64]

Readers count can change at any time, because acquiring the read lock uses fetch_add and then checks if it was valid and undoes if necessary.

[Ericson2314]

Ah true. There might still be a way to optimize it; maybe self.lock.fetch_add(READER, Ordering::Relaxed);, but we can worry about that later.

[64]

yeah it's possible that relaxed ordering is fine there. shouldn't be a problem as long as the add happens before the drop, and the drop happens before the user can access the returned guard. i'll fix the other issue and push later today

[64]

side note: how would you feel about marking RAII guards as must_use? because otherwise they just unlock instantly.

[Ericson2314]

Oh sorry, I meant to approve of that, but fat-fingered it on my phone.

[Ericson2314]

Also keep in mind while I wrote the original RwLock, @mvdnes still needs to sign off on everything and alone can merge.

[64]

I would strongly recommend yanking previous versions of the crate which contain the bug (which looks to be every version of the crate for the last 5 years, unfortunately).

[vinaychandra]

Hi @64 can you please add a few lines in the document commenting about writer contention? Each of the multiple implementations of spinlock in folly have their own considerations (some can starve writers to an extreme for example).

[64]

@vinaychandra How's that?

[vinaychandra]

For example, this is the implementation.

Quoting

Note that the unfairness here is extreme: if the lock is
continually accessed for read, writers will never get a chance

It would be great if we had an option to choose how writer contentions work. We can tweak it so that we have no writer starvation (or atleast reduce the extreme case). (See the other implementations in folly for example)

There is also RWTicketSpinLock which is a bit more fair but slightly less READ performance

Quoting,

RWTicketSpinLock shows more balanced READ/WRITE performance.  If
 *  your application really needs a lot more threads, and a
 *  higher-priority writer, prefer one of the RWTicketSpinLock locks.

[64]

@vinaychandra see my earlier commit. I agree it would be good to have a lock with better write performance. maybe you can submit a PR for that?

[64]

@mvdnes I do apologise for nagging, but is there a chance you could take a look at this PR when you have time? It fixes some pretty dangerous UB in the current RwLock.

[mvdnes]

Sorry for the absence.
This crate has grown quite a bit since it's original conception. Unfortunately I don't have the time and motivation to fully understand these changes.
If you can vouch for this PR than I will merge it, as I do understand it's importance.

[xacrimon]

Hello @mvdnes. Friend of Matt and serious lockfree dude here. I have read thru the changes and tested extensively. It looks good to me.

[mvdnes]

Ok then. Merged!