Skip to content

Commit

Permalink
Auto merge of #51630 - joshlf:map-split-perf, r=dtolnay
Browse files Browse the repository at this point in the history
Optimize RefCell refcount tracking

Address the performance concern raised in #51466 (comment)

cc @dtolnay  @nnethercote @rust-lang/wg-compiler-performance

cc @RalfJung @jhjourdan for soundness concerns

Can somebody kick off a perf run on this? I'm not sure how that's done, but I understand it has to be started manually.

The idea of this change is to switch to representing mutable refcount as values below 0 to eliminate some branching that was required with the old algorithm.
  • Loading branch information
bors committed Jun 28, 2018
2 parents 9f79d2f + 851cc39 commit 5d95db3
Show file tree
Hide file tree
Showing 5 changed files with 40 additions and 34 deletions.
66 changes: 36 additions & 30 deletions src/libcore/cell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -570,20 +570,31 @@ impl Display for BorrowMutError {
}
}

// Values [1, MIN_WRITING-1] represent the number of `Ref` active. Values in
// [MIN_WRITING, MAX-1] represent the number of `RefMut` active. Multiple
// `RefMut`s can only be active at a time if they refer to distinct,
// nonoverlapping components of a `RefCell` (e.g., different ranges of a slice).
// Positive values represent the number of `Ref` active. Negative values
// represent the number of `RefMut` active. Multiple `RefMut`s can only be
// active at a time if they refer to distinct, nonoverlapping components of a
// `RefCell` (e.g., different ranges of a slice).
//
// `Ref` and `RefMut` are both two words in size, and so there will likely never
// be enough `Ref`s or `RefMut`s in existence to overflow half of the `usize`
// range. Thus, a `BorrowFlag` will probably never overflow. However, this is
// not a guarantee, as a pathological program could repeatedly create and then
// mem::forget `Ref`s or `RefMut`s. Thus, all code must explicitly check for
// overflow in order to avoid unsafety.
type BorrowFlag = usize;
// range. Thus, a `BorrowFlag` will probably never overflow or underflow.
// However, this is not a guarantee, as a pathological program could repeatedly
// create and then mem::forget `Ref`s or `RefMut`s. Thus, all code must
// explicitly check for overflow and underflow in order to avoid unsafety, or at
// least behave correctly in the event that overflow or underflow happens (e.g.,
// see BorrowRef::new).
type BorrowFlag = isize;
const UNUSED: BorrowFlag = 0;
const MIN_WRITING: BorrowFlag = (!0)/2 + 1; // 0b1000...

#[inline(always)]
fn is_writing(x: BorrowFlag) -> bool {
x < UNUSED
}

#[inline(always)]
fn is_reading(x: BorrowFlag) -> bool {
x > UNUSED
}

impl<T> RefCell<T> {
/// Creates a new `RefCell` containing `value`.
Expand Down Expand Up @@ -1022,12 +1033,11 @@ impl<'b> BorrowRef<'b> {
#[inline]
fn new(borrow: &'b Cell<BorrowFlag>) -> Option<BorrowRef<'b>> {
let b = borrow.get();
if b >= MIN_WRITING {
if is_writing(b) || b == isize::max_value() {
// If there's currently a writing borrow, or if incrementing the
// refcount would overflow into a writing borrow.
None
} else {
// Prevent the borrow counter from overflowing into
// a writing borrow.
assert!(b < MIN_WRITING - 1);
borrow.set(b + 1);
Some(BorrowRef { borrow })
}
Expand All @@ -1038,7 +1048,7 @@ impl<'b> Drop for BorrowRef<'b> {
#[inline]
fn drop(&mut self) {
let borrow = self.borrow.get();
debug_assert!(borrow < MIN_WRITING && borrow != UNUSED);
debug_assert!(is_reading(borrow));
self.borrow.set(borrow - 1);
}
}
Expand All @@ -1047,12 +1057,12 @@ impl<'b> Clone for BorrowRef<'b> {
#[inline]
fn clone(&self) -> BorrowRef<'b> {
// Since this Ref exists, we know the borrow flag
// is not set to WRITING.
// is a reading borrow.
let borrow = self.borrow.get();
debug_assert!(borrow != UNUSED);
debug_assert!(is_reading(borrow));
// Prevent the borrow counter from overflowing into
// a writing borrow.
assert!(borrow < MIN_WRITING - 1);
assert!(borrow != isize::max_value());
self.borrow.set(borrow + 1);
BorrowRef { borrow: self.borrow }
}
Expand Down Expand Up @@ -1251,12 +1261,8 @@ impl<'b> Drop for BorrowRefMut<'b> {
#[inline]
fn drop(&mut self) {
let borrow = self.borrow.get();
debug_assert!(borrow >= MIN_WRITING);
self.borrow.set(if borrow == MIN_WRITING {
UNUSED
} else {
borrow - 1
});
debug_assert!(is_writing(borrow));
self.borrow.set(borrow + 1);
}
}

Expand All @@ -1266,10 +1272,10 @@ impl<'b> BorrowRefMut<'b> {
// NOTE: Unlike BorrowRefMut::clone, new is called to create the initial
// mutable reference, and so there must currently be no existing
// references. Thus, while clone increments the mutable refcount, here
// we simply go directly from UNUSED to MIN_WRITING.
// we explicitly only allow going from UNUSED to UNUSED - 1.
match borrow.get() {
UNUSED => {
borrow.set(MIN_WRITING);
borrow.set(UNUSED - 1);
Some(BorrowRefMut { borrow: borrow })
},
_ => None,
Expand All @@ -1284,10 +1290,10 @@ impl<'b> BorrowRefMut<'b> {
#[inline]
fn clone(&self) -> BorrowRefMut<'b> {
let borrow = self.borrow.get();
debug_assert!(borrow >= MIN_WRITING);
// Prevent the borrow counter from overflowing.
assert!(borrow != !0);
self.borrow.set(borrow + 1);
debug_assert!(is_writing(borrow));
// Prevent the borrow counter from underflowing.
assert!(borrow != isize::min_value());
self.borrow.set(borrow - 1);
BorrowRefMut { borrow: self.borrow }
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/compile-fail/not-panic-safe-2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,5 @@ fn assert<T: UnwindSafe + ?Sized>() {}
fn main() {
assert::<Rc<RefCell<i32>>>();
//~^ ERROR the type `std::cell::UnsafeCell<i32>` may contain interior mutability and a
//~| ERROR the type `std::cell::UnsafeCell<usize>` may contain interior mutability and a
//~| ERROR the type `std::cell::UnsafeCell<isize>` may contain interior mutability and a
}
2 changes: 1 addition & 1 deletion src/test/compile-fail/not-panic-safe-3.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,5 @@ fn assert<T: UnwindSafe + ?Sized>() {}
fn main() {
assert::<Arc<RefCell<i32>>>();
//~^ ERROR the type `std::cell::UnsafeCell<i32>` may contain interior mutability and a
//~| ERROR the type `std::cell::UnsafeCell<usize>` may contain interior mutability and a
//~| ERROR the type `std::cell::UnsafeCell<isize>` may contain interior mutability and a
}
2 changes: 1 addition & 1 deletion src/test/compile-fail/not-panic-safe-4.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,5 @@ fn assert<T: UnwindSafe + ?Sized>() {}
fn main() {
assert::<&RefCell<i32>>();
//~^ ERROR the type `std::cell::UnsafeCell<i32>` may contain interior mutability and a
//~| ERROR the type `std::cell::UnsafeCell<usize>` may contain interior mutability and a
//~| ERROR the type `std::cell::UnsafeCell<isize>` may contain interior mutability and a
}
2 changes: 1 addition & 1 deletion src/test/compile-fail/not-panic-safe-6.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,5 @@ fn assert<T: UnwindSafe + ?Sized>() {}
fn main() {
assert::<*mut RefCell<i32>>();
//~^ ERROR the type `std::cell::UnsafeCell<i32>` may contain interior mutability and a
//~| ERROR the type `std::cell::UnsafeCell<usize>` may contain interior mutability and a
//~| ERROR the type `std::cell::UnsafeCell<isize>` may contain interior mutability and a
}

0 comments on commit 5d95db3

Please sign in to comment.