Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add into_inner and get_mut to Mutex and RwLock #29031

Merged
merged 2 commits into from
Oct 16, 2015

Conversation

cristicbz
Copy link
Contributor

The implementation for into_inner was a bit more complex than I had hoped for---is there any simpler, less unsafe way of getting around the fact that one can't move out of a Drop struct?

See #28968 and rust-lang/rfcs#1269 .

@rust-highfive
Copy link
Collaborator

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@cristicbz
Copy link
Contributor Author

cc @gankro @tomaka @alexcrichton

// The following code is like `let Mutex { inner, data } = self`.
let mut inner = mem::uninitialized::<Box<StaticMutex>>();
let mut data = mem::uninitialized::<UnsafeCell<T>>();
ptr::copy_nonoverlapping(&self.inner as *const Box<StaticMutex>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just use ptr::read

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also note that &T coerces to *T

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. (re: coercion, just doing &self.inner didn't type check for some reason---maybe some interaction with type inference.)

@Gankra
Copy link
Contributor

Gankra commented Oct 13, 2015

Other than the ptr::read stuff, this looks great! Really thorough. I'm not really qualified to review this kind of code, though.

@cristicbz
Copy link
Contributor Author

Thanks @gankro! I changed to ptr::read, I'd forgotten about that one.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 13, 2015
@cristicbz
Copy link
Contributor Author

Local (full) make check just finished and passed.

@cristicbz
Copy link
Contributor Author

@aturon mentioned in a different bug he is going to be unavailable. r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned aturon Oct 14, 2015
@brson brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 14, 2015
@huonw
Copy link
Member

huonw commented Oct 14, 2015

This shouldn't close #28968 if it is using it as a tracking issue.

@alexcrichton
Copy link
Member

The libs team talked about this today, and the conclusion is that this is good to go. The changes we'd like to see, however, are:

  • Could you add these methods to RwLock as well? They'd basically all be doing the same thing.
  • It may be good to get some exhaustive matching here to ensure that all struct fields are moved out of when into_inner is called. For example something like:
let (a, b) = {
    let Mutex { ref a, ref b } = self; // note exhaustive match
    (ptr::read(a), ptr::read(b))
};
mem::forget(self);
  • Could you add a comment in the destructor as well indicating that into_inner needs to be updated with any changes as well?

@cristicbz
Copy link
Contributor Author

@huonw Good point, I edited the description to not close the issue.

@alexcrichton That all makes sense, I'll do all of that when I get back from work, thanks!

@cristicbz
Copy link
Contributor Author

@alexcrichton I implemented everything you asked for, please take a look.

Thanks!

///
/// If another user of this mutex panicked while holding the mutex, then
/// this call will return an error instead.
#[inline]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah as one final piece, could these #[inline] annotations be left out? These are pretty hefty functions and the inline shouldn't be needed anyway as a result of the generic parameter on Mutex

@cristicbz
Copy link
Contributor Author

@alexcrichton Done and squashed. Don't feel strongly, but I had added #[inline] before because (a) they're not actually that big (especially get_mut) and (b) consistency with the other methods on the types. That said I don't expect these methods to be called in a tight loop (event get_mut), so this is probably for the better.

The fact that it has a type param doesn't quite imply #[inline], right? It's still a hint for LLVM---I remember when we inlined all the slice methods (also type param'd) and it made a massive impact on perf.

@cristicbz cristicbz changed the title Add into_inner and get_mut to sync::Mutex Add into_inner and get_mut to Mutex and RwLock Oct 15, 2015
@alexcrichton
Copy link
Member

@bors: r+ 90ccefd

Thanks!

Yeah #[inline] is an extra hint on top of monomorphizing to LLVM but it rarely has a massive impact on perf for generic functions. The real impact it has is when a function is otherwise not translated across crates (e.g. a concrete function like str::len).

@bors
Copy link
Contributor

bors commented Oct 15, 2015

⌛ Testing commit 90ccefd with merge 6cdf31b...

bors added a commit that referenced this pull request Oct 15, 2015
The implementation for `into_inner` was a bit more complex than I had hoped for---is there any simpler, less unsafe way of getting around the fact that one can't move out of a `Drop` struct?

See #28968 and rust-lang/rfcs#1269 .
@bors bors merged commit 90ccefd into rust-lang:master Oct 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants