Add support for RwLock #88

pop · 2019-10-29T01:06:08Z

Fixes #21

Oh snap when did this move to the Tokio org? Well, all the same: I added support for RwLock!

I don't know if this is done, but the tests (which I got from the RwLock docs) pass so I'm happy.

I would love some feedback. The is_read_locked() and is_write_locked() functions cause "Already Locked" panics so I am not using them at the moment. Let me know if you want more info on that.

cc: @pickfire ?

src/rt/rwlock.rs

pickfire · 2019-10-30T16:54:16Z

@pop Nice, I believe I may be lack of experience to review this but probably @hawkw could help.

hawkw

The implementation looks good to me, though I'd want to get @carllerche's opinion as well — I think he knows loom's internals much better than I do.

I left comments on some minor nits.

.gitignore

src/rt/rwlock.rs

hawkw · 2019-10-30T17:39:24Z

src/rt/rwlock.rs

+    seq_cst: bool,
+
+    /// `Some` when the rwlock is in the locked state.
+    /// The `thread::Id` references the thread that currently holds the rwlock.


I think this comment could be made clearer, since the lock value could either be a single thread ID holding the write lock, or multiple thread IDs holding read locks.

src/rt/rwlock.rs

hawkw · 2019-10-30T17:48:08Z

src/rt/rwlock.rs

+                execution.threads.seq_cst();
+            }
+
+            // Block all other threads attempting to acquire rwlock


Nit/TIOLI: seems like this code is repeated enough that it could maybe be factored out into its own function...not a blocker.

src/rt/rwlock.rs

hawkw · 2019-10-30T17:50:15Z

src/rt/rwlock.rs

+
+impl RwLock {
+    /// Common RwLock function
+    pub(crate) fn new(seq_cst: bool) -> RwLock {


As far as I can tell, the rt::RwLock type is only ever constructed in sync::RwLock::new, and it always passes true here. Will we ever construct a rt::RwLock with seq_cst=false? If not, should we just remove the bool and always establish sequential consistency?

hawkw · 2019-10-30T17:51:22Z

src/sync/rwlock.rs

+#[derive(Debug)]
+pub struct RwLockReadGuard<'a, T> {
+    lock: &'a RwLock<T>,
+    data: Option<std::sync::RwLockReadGuard<'a, T>>,


It's not immediately obvious why this is an Option. I think it's it so that the Drop impl can drop the std guard and release the std lock before releasing the loom mock lock, because that might cause another thread to acquire the lock?

I think a comment explaining this (either here on the field or in the Drop impl) would be very helpful.

pop

Thanks for the feedback @hawkw. I'll get to these fixes ASAP!

pop · 2019-10-31T20:14:01Z

I'm taking a second look at some of what I implemented. In trying to factor out some code I think I found a bug in my implementation. I'll remove the WIP title when this is ready for review again.

pop · 2019-11-02T23:40:40Z

This is ready for a final review... or at least I think it is right now... so throw your feedback at me (again)!

Some feedback I haven't taken:

A part of the code looks like I should be able to edit a HashMap but instead I create a new one. I need a bit more hand-holding to get that refactor done. I don't know how to make rustc happy. When I end up putting lifetime specifiers in stuff that is three steps removed from the change I'm making I start to think "this is not a good refactor".
There's some repeated code. I refactored out that loop and a similar loop for symmetry. I'm not super sure about that choice, but the loops had a bug so I figured putting them close to eachother in the code might make it easier to contrast the two.

hawkw · 2019-11-03T01:12:10Z

@pop re:

A part of the code looks like I should be able to edit a HashMap but instead I create a new one. I need a bit more hand-holding to get that refactor done. I don't know how to make rustc happy.

what do you think about something like this:

            // Set the lock to the current thread
            let mut already_locked = false;
            state.lock = match state.lock.take() {
                None => {
                    let mut threads: HashSet<thread::Id> = HashSet::new();
                    threads.insert(thread_id);
                    Some(Locked::Read(threads))
                }
                Some(Locked::Read(threads)) => {

                    threads.insert(thread_id);
                    Some(Locked::Read(threads))
                }
                Some(Locked::Write(writer)) => {
                    already_locked = true;
                    Some(Locked::Write(writer))
                }
            };

            if already_locked {
                return false;
            }

pickfire · 2019-11-03T09:53:57Z

src/rt/rwlock.rs

+pub(super) struct State {
+    /// A single `thread::Id` when Write locked.
+    /// A set of `thread::Id` when Read locked.
+    lock: Option<Locked>,


Why does this have to be Option<Locked>? Could it be Locked and initialized at first use?

If no threads are reading or writing the RwLock then it doesn't make sense for it to be in either Locked::Read or Locked::Write, so I think None makes the most sense.

Ah, so similar to the following

#[derive(Debug, PartialEq)] enum Locked { Read(HashSet<thread::Id>), Write(thread::Id), None, }

I guess Option<Locked> makes more sense now but I wonder if the size is any different from Option<Locked> compared to Locked.

Hmm. I don't know about that. I was more focused on the expressiveness of the implementation and not the size impact. I can change it to your suggestion if we're concerned about it. Your suggestion seems pretty expressive too -- "the RwLock can be in one of three states..." makes sense too.

No, I am not sure whether it affects the size, just thought about it, we need to verify it with probably something like std::mem::size_of<Type>().

Is this the kind of check you meant?

I added this print statement locally to rwlock's new() method:

println!("sizeof Option<Locked> {:?}", std::mem::size_of::<Option<Locked>>()); println!("sizeof Locked {:?}", std::mem::size_of::<Locked>());

and got this output:

sizeof Option<Locked> 64 sizeof Locked 64

Which makes me think both are smaller than the smallest memory allocation we can claim.

This was on Linux 5.2.18-200.fc30.x86_64 FWIW.

Oh, I thought they have different sizes.

pop · 2019-11-03T14:51:18Z

Thanks @hawkw and @pickfire for the review(s)! I'm learning a lot 😄

src/rt/object.rs

tests/mutex.rs

src/rt/rwlock.rs

pop · 2019-11-05T17:27:32Z

I think we're just waiting on a review from @carllerche, is that right @hawkw @pickfire?

hawkw · 2019-11-05T18:54:52Z

I think we're just waiting on a review from @carllerche, is that right @hawkw @pickfire?

@pop as far as I know, that's right! This looks good to me!

pickfire · 2019-11-06T17:01:29Z

Yup, looks good to me too.

Refs tokio-rs#21

Problem: `is_read_locked` and `is_write_locked` functions cause an "Already Borrowed" error. This implements a workaround, but it'd be good to have that sorted out before calling loom::rwlock "Done". Refs tokio-rs#21

Refs tokio-rs#21

@hawkw

Previously I was doing something like this: ``` state.lock = match &state.lock { None => { create and return new Read lock } Some(existing Read lock) => { Copy existing Read lock contents into new Read lock. Extend new Read lock and return that } ... } ``` This was not performant, but by using `&state.lock` we would have had to add lifetime specifiers _everywhere_. @hawkw shared with me the _very useful_ Option method `take()` which takes an Option's value and leaves None. This pretty much solved the problem: ``` state.lock = match state.lock.take() { None => { create and return new Read lock } Some(existing Read lock) { return extended read lock } ... } ``` Yay! Refs tokio-rs#21

pop · 2020-01-01T21:55:25Z

Rebased against master so there shouldn't be any conflicts. Thanks @hawkw (via #105)!

carllerche · 2020-04-08T20:46:42Z

I merged master and fixed the conflicts 👍

carllerche

LGTM, i'll merge once CI passes.

carllerche · 2020-04-08T21:33:08Z

Sorry for the delay, thanks for doing the work!

carllerche · 2020-04-08T21:33:32Z

Btw, there is a loom channel on the Tokio discord (https://discord.gg/tokio). If something is blocked on me, feel free to ping me there.

pickfire reviewed Oct 30, 2019

View reviewed changes

src/rt/rwlock.rs Outdated Show resolved Hide resolved

hawkw reviewed Oct 30, 2019

View reviewed changes

pop commented Oct 31, 2019

View reviewed changes

pop force-pushed the feature/rwlock branch from e1988f3 to b4a9f30 Compare October 31, 2019 14:56

pop changed the title ~~Add support for RwLock~~ WIP: Add support for RwLock Oct 31, 2019

pop force-pushed the feature/rwlock branch 3 times, most recently from 836facf to 0349e9b Compare November 2, 2019 23:39

pickfire reviewed Nov 3, 2019

View reviewed changes

pop force-pushed the feature/rwlock branch from 099c72d to 34df9cc Compare November 3, 2019 14:50

pickfire reviewed Nov 4, 2019

View reviewed changes

src/rt/object.rs Outdated Show resolved Hide resolved

pickfire reviewed Nov 4, 2019

View reviewed changes

tests/mutex.rs Outdated Show resolved Hide resolved

pop changed the title ~~WIP: Add support for RwLock~~ Add support for RwLock Nov 4, 2019

pop force-pushed the feature/rwlock branch from 34df9cc to cd84f9d Compare November 4, 2019 17:29

pickfire reviewed Nov 4, 2019

View reviewed changes

src/rt/rwlock.rs Outdated Show resolved Hide resolved

hawkw mentioned this pull request Dec 18, 2019

Add support for RwLock #105

Closed

pop added 4 commits January 1, 2020 13:45

Feature(rwlock): Tests

3ebb83e

Refs tokio-rs#21

Feature(rwlock): Passing rwlock tests!

1def516

Problem: `is_read_locked` and `is_write_locked` functions cause an "Already Borrowed" error. This implements a workaround, but it'd be good to have that sorted out before calling loom::rwlock "Done". Refs tokio-rs#21

Feature(RwLock): PR feedback.

97ea3a7

Refs tokio-rs#21

pop force-pushed the feature/rwlock branch from cd84f9d to b4b7ad0 Compare January 1, 2020 21:54

vakaras mentioned this pull request Apr 8, 2020

Add support for mpsc channels #118

Merged

Merge remote-tracking branch 'origin/master' into feature/rwlock

c90f3c6

carllerche approved these changes Apr 8, 2020

View reviewed changes

carllerche merged commit e71deed into tokio-rs:master Apr 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for RwLock #88

Add support for RwLock #88

pop commented Oct 29, 2019 •

edited

pickfire commented Oct 30, 2019

hawkw left a comment

hawkw Oct 30, 2019

hawkw Oct 30, 2019

hawkw Oct 30, 2019

hawkw Oct 30, 2019

pop left a comment

pop commented Oct 31, 2019

pop commented Nov 2, 2019

hawkw commented Nov 3, 2019

pickfire Nov 3, 2019

pop Nov 3, 2019

pickfire Nov 4, 2019

pop Nov 4, 2019

pickfire Nov 4, 2019

pop Nov 5, 2019 •

edited

pickfire Nov 5, 2019

pop commented Nov 3, 2019

pop commented Nov 5, 2019

hawkw commented Nov 5, 2019 •

edited

pickfire commented Nov 6, 2019

pop commented Jan 1, 2020

carllerche commented Apr 8, 2020

carllerche left a comment

carllerche commented Apr 8, 2020

carllerche commented Apr 8, 2020

Add support for RwLock #88

Add support for RwLock #88

Conversation

pop commented Oct 29, 2019 • edited

pickfire commented Oct 30, 2019

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pop left a comment

Choose a reason for hiding this comment

pop commented Oct 31, 2019

pop commented Nov 2, 2019

hawkw commented Nov 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pop Nov 5, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pop commented Nov 3, 2019

pop commented Nov 5, 2019

hawkw commented Nov 5, 2019 • edited

pickfire commented Nov 6, 2019

pop commented Jan 1, 2020

carllerche commented Apr 8, 2020

carllerche left a comment

Choose a reason for hiding this comment

carllerche commented Apr 8, 2020

carllerche commented Apr 8, 2020

pop commented Oct 29, 2019 •

edited

pop Nov 5, 2019 •

edited

hawkw commented Nov 5, 2019 •

edited