Safer word lock

[faern]

I tried to reason a bit about the invariants of the unsafe code in WordLock. I think we can get away with them not being unsafe with some minor adjustments. Mostly this PR removes unsafe keywords from methods and adds unsafe blocks within them. But there is one more difference that I'm hoping should make this safe to call from the outside:

fn unlock(&self) {
    let state = self.state.fetch_and(!LOCKED_BIT, Ordering::Release);
    ...
}

Switching from fetch_sub to fetch_and is intended to make the method not corrupt self.state if called on an already unlocked WordLock. Previously multiple calls would continue to decrement the integer by one, locking and unlocking it as well as corrupting the part of it that is used as a pointer. By using the bit and operation this should just zero out the LOCKED_BIT without touching the rest. The test suite passes, I'm not sure if I have missed something else that makes this a bad change.

If you don't like the trait on usize providing the convenience methods we can skip them. They are unrelated to making the methods safe, other than it might be harder to screw code up if the bit manipulation is extracted.

[Amanieu]

There is a good reason for using fetch_sub instead of fetch_and: it results in better code generation on x86 since it allows the use of the xadd instruction. fetch_and on the other hand requires a compare-exchange loop.

Code

[Amanieu]

This doesn't modify self, so maybe with_queue_head is a better name?

[faern]

That makes sense. Will also make every method consume self since it's just a Copy type anyway.

[faern]

Ah, crap. I suspected that was the reason. So I will make lock unsafe again and keep lock_slow safe. Still way less code under unsafe.

[Amanieu]

OK, so this is a bit subtle, but I would prefer if f was only called from one place and you simply extracted a reference from the thread-local storage. The reason for this is that if f is used in 2 different places, it will effectively be inlined twice into the function, which doubles the code size. This will require a bit of unsafe code to juggle the lifetimes.

[faern]

Ah, yeah. Makes sense. I did this now, but without much unsafe. Instead of juggling too many pointers I just stored the reference or owned value in a Cow<ThreadData>

[Amanieu]

You can use Option::get_or_insert_with instead of this if block.

[faern]

I don't see how that would work here. thread_data_storage is always None when reaching this if block. So the closure to get_or_insert_with would always run and I would still need to get the address out for my pointer variable later. My initial declaration of thread_data_storage was a bit off, but I moved it now to better show how it's used.

[Amanieu]

thread_data_ptr = thread_data_storage.get_or_insert_with(ThreadData::new);

Since get_or_insert_with returns a reference to the inner value in the option, you can avoid calling as_ref and unwrap.

[faern]

🤦‍♂️ I completely missed it returns a reference. Of course it does.

[Amanieu]

Thanks!