fitzgen / bumpalo

A fast bump allocation arena for Rust
https://docs.rs/bumpalo
Apache License 2.0
1.39k stars 110 forks source link

Could `Bump` be `Sync`? #53

Closed rtfeldman closed 4 years ago

rtfeldman commented 4 years ago

Thanks for making a lovely library!

I don't know enough about the internals to know if Bump could safely implement Sync (the way it now does for Send) but I recently found myself wanting to share a single &Bump between threads, so that several threads could allocate into the same arena in parallel, and then once all the threads had been joined, there could be additional processing done on the arena before dropping it.

Is it conceivable that Bump could implement Sync as well as Send? Or is there maybe a technique I'm not thinking of where it's possible to achieve the same result (multiple threads bump-allocating in parallel, and then using that memory on the main thread once the threads have completed) without Bump implementing Sync?

fitzgen commented 4 years ago

Thanks!

Bump uses Cell underneath the covers, and doesn't have any synchronization of its own, so we can't implement Sync for it directly. You can wrap the Bump in a Mutex to make it Sync, but it would probably be better to give each thread its own Bump if possible.

Or is there maybe a technique I'm not thinking of where it's possible to achieve the same result (multiple threads bump-allocating in parallel, and then using that memory on the main thread once the threads have completed) without Bump implementing Sync?

Because &mut T is Send if T: Send and Bump is Send, you can allocate Bumps on the main thread, send &mut Bumps from there to the worker threads, and have them allocate out of it that way. Does that work for you?

rtfeldman commented 4 years ago

Makes sense, thank you for the info!

sesse commented 2 years ago

I'm also hitting this, but for a completely different reason; I've made a HashMap that allocates its keys on a Bump, and I want to share that table read-only between threads. So I don't want to allocate anything more on the Bump, but since the HashMap's key's lifetimes are tied to that of the Bump, I do need to send it to the other threads… and thus the requirement to Sync.

The code looks roughly like this:

struct S {
    arena: Bump,

    #[covariant]
    #[borrows(arena)]
    ht: HashMap<&'this [u8], Value>,
}

and since I can't send ht alone to other threads with an anonymous lifetime, I'd have to send all of S, and then S needs to be Sync. (Wrapping S in Arc<Mutex<…>> would work, but would kill concurrency. Using an RwLock needs Bump to be Sync, again.)

Am I missing something? :-)

fitzgen commented 2 years ago

@sesse you're already doing unsafe things with the self-borrows, so I'd suggest just doing a little more unsafe to wrap Bump in a new type that is specifically only for use within S and it is therefore easy to locally guarantee the safety of a few unsafe operations:

struct SBump(Bump);

impl SBump {
    /// SAFETY: it is the caller's responsibility to ensure that `self`
    /// is not shared across threads during this call (eg it is called
    /// within a `&mut self` method on `S`).
    pub unsafe fn alloc_key(&self, key: &[u8]) -> &[u8] { self.0.alloc_slice_copy(key) }
}

// SAFETY: these are okay, if callers follow the documented safety
// requirements for `SBump`'s unsafe methods.
unsafe impl Send for SBump {}
unsafe impl Sync for SBump {}