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

Collapse the 'rooted' field into the ptr #17

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 51 additions & 16 deletions gc/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,14 @@
//! It is marked as non-sendable because the garbage collection only occurs
//! thread locally.

#![feature(borrow_state, coerce_unsized, core, optin_builtin_traits, nonzero, unsize)]
#![feature(borrow_state, core, optin_builtin_traits, nonzero)]

extern crate core;

use core::nonzero::NonZero;
use gc::GcBox;
use std::cell::{self, Cell, RefCell, BorrowState};
use std::ops::{Deref, DerefMut, CoerceUnsized};
use std::cell::{self, Cell, RefCell, BorrowState, UnsafeCell};
use std::ops::{Deref, DerefMut};
use std::marker;

mod gc;
Expand All @@ -22,6 +22,8 @@ mod trace;
pub use trace::Trace;
pub use gc::force_collect;

const EVERYTHING_BUT_LEAST_SIGNIFICANT_BIT: usize = ::std::usize::MAX - 1;

////////
// Gc //
////////
Expand All @@ -30,17 +32,13 @@ pub use gc::force_collect;
///
/// See the [module level documentation](./) for more details.
pub struct Gc<T: Trace + ?Sized + 'static> {
// XXX We can probably take advantage of alignment to store this
root: Cell<bool>,
_ptr: NonZero<*mut GcBox<T>>,
_ptr: UnsafeCell<NonZero<*mut GcBox<T>>>,
Copy link
Owner

Choose a reason for hiding this comment

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

Are we sure that it's safe to use both NonZero and fiddle with the low bit at once? Looks safe, but it might become unsafe after optimizations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it should be fine. In a case where the compiler uses it to store other data, you won't be able to flip the bit, and in a case where it has legitimate (non-zero) data, flipping the bottom bit won't have any impact on the "zeroness" because of the pointer alignment, the bottom bit wouldn't be the deciding factor on if the pointer is zeroed or not.

That said, I'm not certain what kind of optimizations the compiler does with NonZero.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not concerned with cases where the compiler uses it to store other data; I'm concerned with the optimization hints that LLVM gets and the ordering of the flips.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I have no idea then

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty sure that it's fine, but I'd also be OK with dropping it until we find someone who would know better. The impression I have about NonZero, is that it is used as a marker trait for Zeroable types which guarantees that the zero value of the type may be used as a marker value for variants. I don't think that this information is really communicated to llvm that much, and also none of the computations which I've seen in the patch I believe will ever cause problems with setting something to zero.

}

impl<T: ?Sized> !marker::Send for Gc<T> {}

impl<T: ?Sized> !marker::Sync for Gc<T> {}

impl<T: Trace + ?Sized + marker::Unsize<U>, U: Trace + ?Sized> CoerceUnsized<Gc<U>> for Gc<T> {}

impl<T: Trace> Gc<T> {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't like losing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR is broken up into two commits. The first one leaves CoerceUnsigned, but doesn't have UnsafeCell.

I don't know how to get one without removing the other. Maybe it can be done? Maybe it needs support in the standard lib?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe that the CoerceUnsized constraints are a bit too strong in rustc. I believe NonZero allows for the CoerceUnsize because of this: https://github.com/rust-lang/rust/blob/f9b8c49cdb75fb571c7b3ea4af90b0e96929276c/src/libcore/nonzero.rs#L78. UnsafeCell doesn't have such an impl for it (although I believe it would be safe for it to have one).

/// Constructs a new `Gc<T>`.
///
Expand All @@ -63,15 +61,48 @@ impl<T: Trace> Gc<T> {
// When we create a Gc<T>, all pointers which have been moved to the
// heap no longer need to be rooted, so we unroot them.
(**ptr).value().unroot();
Gc { _ptr: ptr, root: Cell::new(true) }
let gc = Gc { _ptr: UnsafeCell::new(ptr) };
gc.set_rooted(true);
gc
}
}
}

impl<T: Trace + ?Sized> Gc<T> {
#[inline]
fn inner(&self) -> &GcBox<T> {
unsafe { &**self._ptr }
use std::mem::transmute;
unsafe {
let gc_box: *mut GcBox<_> = **self._ptr.get();
let ptrs: *mut usize = transmute(&gc_box);
*ptrs &= EVERYTHING_BUT_LEAST_SIGNIFICANT_BIT;
transmute::<*mut GcBox<_>, &mut GcBox<_>>(gc_box)
}
}

fn is_rooted(&self) -> bool {
use std::mem::transmute;
unsafe {
let gc_box: &*mut GcBox<T> = &**self._ptr.get();
// Address of gc_box on the stack.
let ptrs: *mut usize = transmute::<&*mut _, _>(gc_box);
*ptrs & 1 == 1
}
}

#[allow(mutable_transmutes)]
fn set_rooted(&self, b: bool) {
use std::mem::transmute;
unsafe {
let gc_box: &*mut GcBox<T> = &**self._ptr.get();
// Address of gc_box on the stack.
let ptrs: *mut usize = transmute::<&*mut _, _>(gc_box);

// Clear out the bottom bit
*ptrs = *ptrs & EVERYTHING_BUT_LEAST_SIGNIFICANT_BIT;
// Set the bottom bit
*ptrs = *ptrs | if b { 1 } else { 0 };
}
}
}

Expand All @@ -83,16 +114,16 @@ unsafe impl<T: Trace + ?Sized> Trace for Gc<T> {

#[inline]
unsafe fn root(&self) {
assert!(!self.root.get(), "Can't double-root a Gc<T>");
self.root.set(true);
assert!(!self.is_rooted(), "Can't double-root a Gc<T>");
self.set_rooted(true);

self.inner().root_inner();
}

#[inline]
unsafe fn unroot(&self) {
assert!(self.root.get(), "Can't double-unroot a Gc<T>");
self.root.set(false);
assert!(self.is_rooted(), "Can't double-unroot a Gc<T>");
self.set_rooted(false);

self.inner().unroot_inner();
}
Expand All @@ -102,7 +133,11 @@ impl<T: Trace + ?Sized> Clone for Gc<T> {
#[inline]
fn clone(&self) -> Gc<T> {
unsafe { self.inner().root_inner(); }
Gc { _ptr: self._ptr, root: Cell::new(true) }
let gc = unsafe {
Gc { _ptr: UnsafeCell::new(*self._ptr.get())}
};
gc.set_rooted(true);
gc
}
}

Expand All @@ -119,7 +154,7 @@ impl<T: Trace + ?Sized> Drop for Gc<T> {
#[inline]
fn drop(&mut self) {
// If this pointer was a root, we should unroot it.
if self.root.get() {
if self.is_rooted() {
unsafe { self.inner().unroot_inner(); }
}
}
Expand Down
5 changes: 4 additions & 1 deletion gc/tests/gc_semantics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,8 @@ fn gccell_rooting() {
FLAGS.with(|f| assert_eq!(f.get(), GcWatchFlags::new(3, 1, 2, 1)))
}

// Disabled because UnsafeCell doesn't work with CoerceUnsized
/*
#[test]
fn trait_gc() {
#[derive(Trace)]
Expand All @@ -241,9 +243,10 @@ fn trait_gc() {
assert_eq!(x.f(), 10);
}

let gc_bar = Gc::new(Bar);
let gc_bar: Gc<Bar> = Gc::new(Bar);
let gc_foo: Gc<Foo> = gc_bar.clone();

use_trait_gc(gc_foo);
use_trait_gc(gc_bar);
}
*/