From 851cc39503a53b38ecbdd03bd391289e095bd7ca Mon Sep 17 00:00:00 2001 From: Joshua Liebow-Feeser Date: Wed, 27 Jun 2018 00:07:18 -0700 Subject: [PATCH] Optimize RefCell refcount tracking --- src/libcore/cell.rs | 66 ++++++++++++----------- src/test/compile-fail/not-panic-safe-2.rs | 2 +- src/test/compile-fail/not-panic-safe-3.rs | 2 +- src/test/compile-fail/not-panic-safe-4.rs | 2 +- src/test/compile-fail/not-panic-safe-6.rs | 2 +- 5 files changed, 40 insertions(+), 34 deletions(-) diff --git a/src/libcore/cell.rs b/src/libcore/cell.rs index dd8fce17cff90..21edc6dfee4e5 100644 --- a/src/libcore/cell.rs +++ b/src/libcore/cell.rs @@ -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 RefCell { /// Creates a new `RefCell` containing `value`. @@ -1022,12 +1033,11 @@ impl<'b> BorrowRef<'b> { #[inline] fn new(borrow: &'b Cell) -> Option> { 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 }) } @@ -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); } } @@ -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 } } @@ -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); } } @@ -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, @@ -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 } } } diff --git a/src/test/compile-fail/not-panic-safe-2.rs b/src/test/compile-fail/not-panic-safe-2.rs index d750851b719e4..5490acf4bd6c1 100644 --- a/src/test/compile-fail/not-panic-safe-2.rs +++ b/src/test/compile-fail/not-panic-safe-2.rs @@ -19,5 +19,5 @@ fn assert() {} fn main() { assert::>>(); //~^ ERROR the type `std::cell::UnsafeCell` may contain interior mutability and a - //~| ERROR the type `std::cell::UnsafeCell` may contain interior mutability and a + //~| ERROR the type `std::cell::UnsafeCell` may contain interior mutability and a } diff --git a/src/test/compile-fail/not-panic-safe-3.rs b/src/test/compile-fail/not-panic-safe-3.rs index cd27b274258ea..0fac395a11582 100644 --- a/src/test/compile-fail/not-panic-safe-3.rs +++ b/src/test/compile-fail/not-panic-safe-3.rs @@ -19,5 +19,5 @@ fn assert() {} fn main() { assert::>>(); //~^ ERROR the type `std::cell::UnsafeCell` may contain interior mutability and a - //~| ERROR the type `std::cell::UnsafeCell` may contain interior mutability and a + //~| ERROR the type `std::cell::UnsafeCell` may contain interior mutability and a } diff --git a/src/test/compile-fail/not-panic-safe-4.rs b/src/test/compile-fail/not-panic-safe-4.rs index 956eca432c5f0..bf0392018b5dd 100644 --- a/src/test/compile-fail/not-panic-safe-4.rs +++ b/src/test/compile-fail/not-panic-safe-4.rs @@ -18,5 +18,5 @@ fn assert() {} fn main() { assert::<&RefCell>(); //~^ ERROR the type `std::cell::UnsafeCell` may contain interior mutability and a - //~| ERROR the type `std::cell::UnsafeCell` may contain interior mutability and a + //~| ERROR the type `std::cell::UnsafeCell` may contain interior mutability and a } diff --git a/src/test/compile-fail/not-panic-safe-6.rs b/src/test/compile-fail/not-panic-safe-6.rs index d0ca1db521204..950f0a0b53aec 100644 --- a/src/test/compile-fail/not-panic-safe-6.rs +++ b/src/test/compile-fail/not-panic-safe-6.rs @@ -18,5 +18,5 @@ fn assert() {} fn main() { assert::<*mut RefCell>(); //~^ ERROR the type `std::cell::UnsafeCell` may contain interior mutability and a - //~| ERROR the type `std::cell::UnsafeCell` may contain interior mutability and a + //~| ERROR the type `std::cell::UnsafeCell` may contain interior mutability and a }