sync: Implement map methods of parking_lot fame #2445

udoprog · 2020-04-25T21:56:36Z

I've introduced MappedRwLockReadGuard and MappedRwLockWriteGuard, which are produced by the relevant map and try_map methods.

Generally, this mimics the way MappedRwLock*Guards are implemented in parking_lot. By storing a raw pointer in the guards themselves referencing the mapped data and maintaining type invariants through PhantomData. I didn't try to think too much about this, so if someone has objections I'd love to hear them.

I've also dropped the internal use of ReleasingPermit, since it made the guards unecessarily large. The number of permits that need to be released are already known by the guards themselves, and is instead governed directly in the relevant Drop impls. This has the benefit of making the guards as small as possible, for the non-mapped variants this means a single reference is enough.

fmt::Debug impls have been adjusted to behave exactly like the delegating impls in parking_lot. fmt::Display impls have been added for all guard types which behave the same. This does change the format of debug impls, for which I'm not sure if we provide any guarantees.

Closes #2442 since it duplicates some of those efforts. If we don't want map methods I can resurrect only the relevant changes.

hawkw · 2020-04-30T22:53:01Z

May also want to note that this closes #2471 as well :)

hawkw · 2020-04-30T22:54:31Z

tokio/src/sync/rwlock.rs

+    where
+        F: FnOnce(&T) -> &U,
+    {
+        let data = f(unsafe { &*self.lock.c.get() });


Is it necessary for this to be unsafe? Can we just dereference the guard normally here?

Fixed. Thanks!

@udoprog I believe you can probably remove the usages of unsafe from several other places too using the same technique. You still have several map and try_map functions that use unsafe in the same way you were before.

tokio/src/sync/rwlock.rs

sunjay

Hi @udoprog! I am the author of issue #2471 and PR #2472. Thank you so much for your work on this PR. Using your work as a reference made creating my PR much easier!

As a result of doing that PR, I ended up having a very close look at this one and noticed a few things that should probably be addressed. I hope you don't mind that I left a few comments.

Thanks again! Looking forward to this landing. :)

sunjay · 2020-04-30T23:13:26Z

tokio/src/sync/rwlock.rs

+    /// # }
+    /// ```
+    #[inline]
+    pub fn map<U, F>(self, f: F) -> MappedRwLockReadGuard<'a, U>


The map/try_map methods should probably be associated functions (using e.g. this: Self) instead of methods that take self. This is important to avoid conflicts with methods on the dereferenced type T. The map methods on parking_lot are associated functions too.

(In fact, your documentation on this method even says "This is an associated function".)

Also, are you planning on adding map and try_map associated functions on MappedRwLockReadGuard and MappedRwLockWriteGuard? Without that it will only be possible to map the data once.

Thanks for the pointer. My head just refactored the methods into self without realizing why.

Also, are you planning on adding map and try_map associated functions on MappedRwLockReadGuard and MappedRwLockWriteGuard? Without that it will only be possible to map the data once.

Not at the moment since I personally have no use for it. Feel free to add it in a follow up to this.

I was proof-reading documentation and decided to add associated methods for the mapped variants. Please look them over. Thanks.

sunjay · 2020-05-01T00:08:57Z

tokio/src/sync/rwlock.rs

+// NB: These impls need to be explicit since we're storing a raw pointer.
+// Safety: Stores a raw pointer to `T`, so if `T` is `Sync`, so is the lock
+// guard over `T`.
+unsafe impl<'a, T> Send for MappedRwLockReadGuard<'a, T> where T: Sync {}


Should these be implementing Sync, not Send? I don't know for sure, but it's suspicious given the code around it and the comment above it.

Modified the comment. The impl is indeed supposed to be Send if we want the guard to be sent across .await for Send futures. See if it makes sense to you now.

sunjay · 2020-05-12T16:40:11Z

@udoprog I managed to merge MappedMutexGuard into MutexGuard in #2472, do you think the same could be possible here for the new guards you introduced? Link to the relevant commits

udoprog · 2020-05-12T17:08:57Z

@sunjay Yeah, for sure.

I just want to make sure that if we want to support upgradeable/downgradeable locks at some point we're not subject to soundness issues as commented here and in the documentation for the guard. I haven't wrapped my head around that issue yet though, so just mimicking the API for now.

udoprog · 2020-05-12T17:16:47Z

Eh. So the downgrade API for Mapped*Guard is being deprecated. Maybe not something we should mimic then. I need to look closer at it. 🤔

It is being kept though for RwLockWriteGuard (but not the mapped version) which seems to be an important distinction.

udoprog · 2020-07-09T08:19:15Z

Bumped to latest changes in Tokio.

I still need this, so would like to somehow move it forward. If someone wants to see how it's used: https://github.com/udoprog/OxidizeBot/search?q=try_map&unscoped_q=try_map

hawkw · 2020-07-10T16:19:03Z

With regards to supporting upgrade/downgrade for guards: I'm not sure how much sense this would make with the current implementation of the RwLock and the way fairness is currently ensured.

Upgrading a RwLockReadGuard would mean enqueuing it at the end of the queue to acquire all of the remaining semaphore permits (and thus the write lock) while still holding a single permit for the read lock. This means that if any attempts to acquire the write lock are already in the queue, they will never be able to acquire the lock, since the queued upgrade is currently holding one of the permits. Because permits are only ever assigned to the head of the queue, this means that attempting to upgrade a read lock while any other task is waiting for a write lock would result in a deadlock. We could avoid this issue by having an upgrade method that releases any currently held permits, but this is then morally equivalent to just dropping the read guard and trying to acquire a write guard...meaning a separate upgrade method may not be strictly necessary.

Given that, IMO, I think I'd prefer to merge the change that presents a smaller API surface for now. If we end up changing our minds about this, we'll have the chance to break the API in 0.3, so we could change the map functions to return a different guard type then.

It would be nice to see if @carllerche agrees, though!

udoprog · 2020-07-10T16:37:19Z

I'm fine with either at this point. Removed the other guard types for now.

hawkw

Looks good to me --- sorry for the delay on the review.

tokio/src/sync/rwlock.rs

hawkw · 2020-07-24T16:46:53Z

note that if PR #2548 merges before this (which I suspect it will), we'll need to rebase this branch and update it, since #2548 adds additional uses of ReleasingPermit that will need to be changed.

udoprog · 2020-07-24T17:28:40Z

Yeah. I'm pretty used to rebasing now.

Generally, this mimics the way `MappedRwLock*Guard`s are implemented in `parking_lot`. By storing a raw pointer in the guards themselves referencing the mapped data and maintaining type invariants through `PhantomData`. I didn't try to think too much about this, so if someone has objections I'd love to hear them. I've also dropped the internal use of `ReleasingPermit`, since it made the guards unecessarily large. The number of permits that need to be released are already known by the guards themselves, and is instead governed directly in the relevant `Drop` impls. This has the benefit of making the guards as small as possible, for the non-mapped variants this means a single reference is enough. `fmt::Debug` impls have been adjusted to behave exactly like the delegating impls in `parking_lot`. `fmt::Display` impls have been added for all guard types which behave the same. This does change the format of debug impls, for which I'm not sure if we provide any guarantees.

udoprog · 2020-08-16T23:53:50Z

Rebased.

@carllerche I think everyone is waiting for an OK from someone because of the API addition, and I think that's you? But not sure. Could you have a look?

udoprog force-pushed the rwlock-map branch from f47ff0f to 22aab21 Compare April 26, 2020 09:17

udoprog changed the title ~~sync: Implement map methods by parking_lot fame~~ sync: Implement map methods of parking_lot fame Apr 26, 2020

udoprog force-pushed the rwlock-map branch 2 times, most recently from ae9d6ef to dd7198b Compare April 26, 2020 12:33

Darksonn added A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-sync Module: tokio/sync S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 28, 2020

Darksonn requested a review from hawkw April 29, 2020 10:14

hawkw mentioned this pull request Apr 30, 2020

Add map method to MutexGuard, RwLockReadGuard, RwLockWriteGuard #2471

Closed

hawkw reviewed Apr 30, 2020

View reviewed changes

sunjay mentioned this pull request May 1, 2020

Added map associated function to MutexGuard that uses new MappedMutexGuard type #2472

Merged

sunjay reviewed May 1, 2020

View reviewed changes

udoprog force-pushed the rwlock-map branch 3 times, most recently from 89b46b8 to b4d2d25 Compare May 7, 2020 19:41

udoprog force-pushed the rwlock-map branch from b4d2d25 to bf38e04 Compare May 12, 2020 04:32

udoprog mentioned this pull request May 14, 2020

Build Fails with tokio revspec not found udoprog/OxidizeBot#68

Closed

udoprog force-pushed the rwlock-map branch from bf38e04 to a04c9d8 Compare May 14, 2020 17:53

udoprog force-pushed the rwlock-map branch from a04c9d8 to 0e41c20 Compare July 9, 2020 08:16

udoprog force-pushed the rwlock-map branch from 0e41c20 to bae2a6a Compare July 9, 2020 10:59

udoprog force-pushed the rwlock-map branch from 2afef98 to 561ecd1 Compare July 10, 2020 16:40

hawkw approved these changes Jul 10, 2020

View reviewed changes

tokio/src/sync/rwlock.rs Outdated Show resolved Hide resolved

tokio/src/sync/rwlock.rs Outdated Show resolved Hide resolved

tokio/src/sync/rwlock.rs Outdated Show resolved Hide resolved

tokio/src/sync/rwlock.rs Outdated Show resolved Hide resolved

hawkw requested a review from carllerche July 10, 2020 17:30

udoprog force-pushed the rwlock-map branch 4 times, most recently from 51bdc6f to 3812179 Compare July 24, 2020 04:37

udoprog force-pushed the rwlock-map branch from 3812179 to ce06c10 Compare August 3, 2020 18:04

udoprog force-pushed the rwlock-map branch from ce06c10 to d2631ca Compare August 16, 2020 23:46

udoprog mentioned this pull request Aug 17, 2020

sync: Implement map methods of parking_lot fame #2771

Merged

Darksonn approved these changes Aug 26, 2020

View reviewed changes

Darksonn removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 26, 2020

Darksonn merged commit 2e7e42b into tokio-rs:master Aug 26, 2020

udoprog deleted the rwlock-map branch August 26, 2020 18:51

Darksonn mentioned this pull request Sep 9, 2020

Write guard to read guard downgrading for sync::RwLock #2733

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sync: Implement map methods of parking_lot fame #2445

sync: Implement map methods of parking_lot fame #2445

udoprog commented Apr 25, 2020 •

edited

hawkw commented Apr 30, 2020

hawkw Apr 30, 2020

udoprog May 1, 2020

sunjay May 1, 2020

sunjay left a comment

sunjay Apr 30, 2020

sunjay Apr 30, 2020

udoprog May 1, 2020

udoprog May 1, 2020

sunjay May 1, 2020

udoprog May 1, 2020

sunjay commented May 12, 2020 •

edited

udoprog commented May 12, 2020 •

edited

udoprog commented May 12, 2020 •

edited

udoprog commented Jul 9, 2020 •

edited

hawkw commented Jul 10, 2020

udoprog commented Jul 10, 2020

hawkw left a comment

hawkw commented Jul 24, 2020

udoprog commented Jul 24, 2020

udoprog commented Aug 16, 2020 •

edited

sync: Implement map methods of parking_lot fame #2445

sync: Implement map methods of parking_lot fame #2445

Conversation

udoprog commented Apr 25, 2020 • edited

hawkw commented Apr 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunjay 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunjay commented May 12, 2020 • edited

udoprog commented May 12, 2020 • edited

udoprog commented May 12, 2020 • edited

udoprog commented Jul 9, 2020 • edited

hawkw commented Jul 10, 2020

udoprog commented Jul 10, 2020

hawkw left a comment

Choose a reason for hiding this comment

hawkw commented Jul 24, 2020

udoprog commented Jul 24, 2020

udoprog commented Aug 16, 2020 • edited

udoprog commented Apr 25, 2020 •

edited

sunjay commented May 12, 2020 •

edited

udoprog commented May 12, 2020 •

edited

udoprog commented May 12, 2020 •

edited

udoprog commented Jul 9, 2020 •

edited

udoprog commented Aug 16, 2020 •

edited