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

Use UnsafeCell in ThreadRng #285

Merged
merged 1 commit into from
Mar 17, 2018

Conversation

pitdicker
Copy link
Contributor

This is based on top of #281, but only the last commit is interesting. Also this PR is in no way mergeable, but I wanted to get your opinion if it even is sensible.

Using UnsafeCell instead of RefCell inside ThreadRng makes the performance overhead of ThreadRng and the ReseedingRng wrapper in-between just about negligible.

Compare:

test thread_rng_u64        ... bench:       4,415 ns/iter (+/- 21) = 1812 MB/s
test gen_u64_hc128         ... bench:       4,185 ns/iter (+/- 32) = 1911 MB/s
test thread_rng_u64        ... bench:       6,950 ns/iter (+/- 71) = 1151 MB/s (master)
test thread_rng_u64        ... bench:       4,988 ns/iter (+/- 40) = 1603 MB/s (BlockRng PR)

The idea is that we currently use RefCell, and panic if a second mutable borrow inside ThreadRng occurs. I can't really think of a situations where that can happen, see the comment in the last commit. In that case it should also be safe to use UnsafeCell directly, with less overhead. But I am not sure what happens during panics...

@pitdicker
Copy link
Contributor Author

@dhardy, or maybe @burdges.

This is actually a small PR, if you think away the commits it is made on top of.
But I am still curious if my reasoning that using UnsafeCell directly in ThreadRng is safe.
I have copied the relevant text from the commit:

When could we get more then two active mutable references to thread_rng?
Normally there are only mutable references to the RNG inside UnsafeCell inside next_u32, next_u64 etc. Within a single thread (which is the definition of ThreadRng), there will only ever be one of these functions active at a time.

A possible scenario where there could be multiple mutable references is if ThreadRng is used inside next_u32 and co. But the three parts that make up ThreadRng, a PRNG, EntropyRng and the ReseedingRng wrapper are all under our control. We just have to ensure none of them use ThreadRng internally, which is nonsensical anyway.

Another possible option might be if we where in the middle of next_u32 (and co.), another thread panics, in this thread destructors start to run, and the destructors make use of ThreadRng. Is this a real problem or does panicking work differently?

So I think the API of RngCore assures we only ever have one mutable reference to the interior of an UnsafeCell in ThreadRng. And if it worked otherwise, we would already have panics in the current code because we now use RefCell.

@burdges
Copy link
Contributor

burdges commented Mar 9, 2018

Actually why Rc<...> instead of UnsafeCell directly? I could imagine ThreadRng reseeding whenever the reference count incremented from 0 to 1, but I'd think a static Rc never hits reference count zero, so this does nothing.

There are a couple minor hiccups with using UnsafeCell directly:

First, you must ensure ThreadRng stays !Send and !Sync like Rc though, because UnsafeCell itself might actually be Send!

Second, I think UnsafeCell is not Clone, so your #[derive(Clone)] would fail, but just impl Clone for ThreadRng manually.

I suppose UnsafeCell<ReseedingRng<Hc128Core, EntropyRng>> contains no Drop types, so maybe ThreadRng: Copy even makes sense. lol At minimum this avoids any Drop code associated to Rc, unless you want to track the reference count yourself for reseeding purposes.

@pitdicker
Copy link
Contributor Author

I was already wondering why the existing code was using Rc<...>. But because it does not impact performance I didn't care yet...

I seems we can't make ThreadRng !Send without using #![feature(optin_builtin_traits)] on nightly, right? Maybe that is the reason for the Rc?

@dhardy
Copy link
Member

dhardy commented Mar 10, 2018

Good point about !Send and !Sync bounds.

As I understand it, there is a "primary instance" of the Reseeding<StdRng> in thread-local memory, and every time you call thread_rng you get a reference to it. I don't know, but are reference to TLS 'static? If not it would be hard to prove correct lifetimes with references into TLS stored in user functions. Unless there's a significant performance overhead I'd prefer to leave the Rc; it's obviously safe where alternatives aren't (e.g. making sure references aren't left dangling during destruction of a thread).

@vks
Copy link
Collaborator

vks commented Mar 12, 2018

@pitdicker

I seems we can't make ThreadRng !Send without using #![feature(optin_builtin_traits)] on nightly, right? Maybe that is the reason for the Rc?

Couldn't you use std::marker::PhantomData with a type that is not Send?

@dhardy dhardy added the P-low Priority: Low label Mar 12, 2018
@pitdicker
Copy link
Contributor Author

Thanks all for the comments. I will clean up this soon(ish).

Couldn't you use std::marker::PhantomData with a type that is not Send?

Ah, nice! So I could use PhantomData with a raw pointer, and not have it take up any memory.

Still I think I'll go with Rc<UnsafeCell<..>> only for this PR, and leave removing the Rc to someone more knowledgeable about the subtleties.

@dhardy dhardy added C-optimisation D-review Do: needs review labels Mar 14, 2018
@dhardy
Copy link
Member

dhardy commented Mar 16, 2018

@pitdicker you can rebase now

@pitdicker
Copy link
Contributor Author

Rebased.

@dhardy
Copy link
Member

dhardy commented Mar 17, 2018

The use of PhantomData with a pointer sounds very hacky to me; I wouldn't like to rely on that. Finally found the tracking issue for feature(optin_builting_traits). Until that lands better just to stick with Rc?

I think I'm happy with the PR as it stands and comments are positive, so merge if you think it's ready.

@pitdicker pitdicker merged commit b84c545 into rust-random:master Mar 17, 2018
@pitdicker pitdicker deleted the threadrng_unsafecell branch March 17, 2018 18:50
pitdicker added a commit that referenced this pull request Apr 4, 2018
@burdges
Copy link
Contributor

burdges commented Sep 9, 2018

I wondered if we should consider an RcRng type that abstracts this beyond just ThreadRng.

let dist = &dist;
let rng = RcRng::new(ChaChaRng::new(...));
let x = dist.sample_iter(rng.clone()).collect();
let y = dist.sample_iter(rng.clone()).collect();

We cannot use UnsafeCell instead of RefCell though because we cannot prevent user supplied methods like next_u32 from referencing us again. See the the Copy requirement on Cell::update. We get away with it here because we control the code being referenced.

We could make Rc<RefCell<R>>: Rng when R: Rng though? Rc is !Sync and !Send as desired. It'd move the RefCell::borrow_mut() calls inside the Rng method calls, thus making the above code work. I'd think RefCell adjusting its borrow counter sounds fairly negligible compared with more Rngs and not forking the Rng improve cache usage. Thoughts?

@dhardy
Copy link
Member

dhardy commented Sep 10, 2018

Check #579; we don't even need Rc in this case.

Yes I think we could make Rc<RefCell<R>>: RngCore where R: RngCore but that's orthogonal. Want to open a PR?

@burdges
Copy link
Contributor

burdges commented Sep 10, 2018

Very nice! We might actually want both

impl<R: RngCore> RngCore for RefCell<R> { .. }
impl<R: RngCore> RngCore for Rc<RefCell<R>> { .. } 

because the first permits simply borrowing the RefCell<R> while the second permits putting it into multiple places.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-optimisation D-review Do: needs review P-low Priority: Low
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants