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

Request: Send + Sync version of a Handle #207

Open
joshlf opened this issue Mar 20, 2018 · 4 comments
Open

Request: Send + Sync version of a Handle #207

joshlf opened this issue Mar 20, 2018 · 4 comments

Comments

@joshlf
Copy link

joshlf commented Mar 20, 2018

For data structures that want to be concurrent and only expose handles, the fact that crossbeam Handles are neither Send nor Sync is an issue. For example, in elfmalloc, we'd like to have allocator handles which are, at the very least, Sync so that you can share them between threads by calling clone from a new thread, but the fact that Handles are not Sync has prevented us from doing that without resorting to unsafe code.

The problem comes down to the fact that Handles use immutable methods in thread-unsafe ways. You could imagine a handle where the pin and is_pinned methods were mutable, leaving clone as the only immutable method, which would be implemented as self.collector().register(). In fact, in order to sidestep this issue, that's exactly what I'm doing right now for elfmalloc:

/// A `Sync` handle on an epoch GC instance.
/// 
/// A `SyncHandle` is a handle on an epoch GC instance which is `Sync`. It is
/// meant to make it easier to work correctly with `Handle`s in a thread-safe
/// manner.
/// 
/// `Handles` are not `Sync` because they keep thread-unsafe references to a
/// `Local` under the hood. `SyncHandle` sidesteps that issue by ensuring that
/// the only immutable method is clone, which is careful not to use the `Handle`
/// stored internally - only the `Arc<Collector>`, which *is* thread-safe.
pub struct SyncHandle {
    handle: Handle,
    collector: Arc<Collector>,
}

impl SyncHandle {
    pub fn new() -> SyncHandle {
        let collector = Arc::new(Collector::new());
        SyncHandle {
            handle: collector.register(),
            collector,
        }
    }

    pub fn pin(&mut self) -> Guard {
        self.handle.pin()
    }
}

impl Clone for SyncHandle {
    fn clone(&self) -> SyncHandle {
        let handle = self.collector.register();
        SyncHandle {
            handle,
            collector: self.collector.clone(),
        }
    }
}

unsafe impl Sync for SyncHandle {}

It would also be nice if such a handle could be Send, although I'm not sure how that would be done with the current Local-based design.

@joshlf
Copy link
Author

joshlf commented Mar 20, 2018

Oh and if you want to put something in a lazy_static!, it has to be both Send and Sync.

@Thomasdezeeuw
Copy link
Contributor

@joshlf
Copy link
Author

joshlf commented Mar 21, 2018

You can, but that has performance implications - every single time you want to use the GC, you have to register a new handle, which is significantly more expensive than pinning because it involves an allocation and an atomic list insertion.

@jeehoonkang
Copy link
Contributor

In #71, we discussed that Handle's clone() is not that practically useful. It seems it's relevant to this issue. At the end of the day, we need a good interface of Handle (or SyncHandle), which accordingly implements clone() or not.

@ghost ghost transferred this issue from crossbeam-rs/crossbeam-epoch Nov 5, 2018
exrook pushed a commit to exrook/crossbeam that referenced this issue Oct 7, 2020
…sbeam-rs#207

Some rustc arguments like --out-dir, -L, and --extern contain absolute paths
which make building the same crate (from crates.io) in different projects
unable to get cache hits despite being otherwise the same. This patch omits
those arguments from the hash calculation which makes sccache much more useful
for local development.

For additional safety we add the cwd to the hash key, since that winds up in
the rlib and otherwise this patch could result in some compiles returning invalid results from cache.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants