From f0b3c02d816253e04e8cc23fb1a7fa342b246689 Mon Sep 17 00:00:00 2001 From: Luca Barbieri Date: Wed, 17 Jul 2019 13:25:34 +0200 Subject: [PATCH 1/3] Optimize RefCell read borrowing Instead of doing two comparisons we can do only one with a bit of cleverness. LLVM currently can't do this optimization itself on x86-64. --- src/libcore/cell.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libcore/cell.rs b/src/libcore/cell.rs index beafddc5a1019..ea803bd3a1fb6 100644 --- a/src/libcore/cell.rs +++ b/src/libcore/cell.rs @@ -1101,13 +1101,13 @@ struct BorrowRef<'b> { impl<'b> BorrowRef<'b> { #[inline] fn new(borrow: &'b Cell) -> Option> { - let b = borrow.get(); - if is_writing(b) || b == isize::max_value() { + let b = borrow.get().wrapping_add(1); + if !is_reading(b) { // If there's currently a writing borrow, or if incrementing the // refcount would overflow into a writing borrow. None } else { - borrow.set(b + 1); + borrow.set(b); Some(BorrowRef { borrow }) } } From c7928f5af0b9dc7b042d4c390a5a5381cc53de9a Mon Sep 17 00:00:00 2001 From: Luca Barbieri Date: Sun, 21 Jul 2019 11:49:36 +0200 Subject: [PATCH 2/3] Expand comment on BorrowRef::new --- src/libcore/cell.rs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/libcore/cell.rs b/src/libcore/cell.rs index ea803bd3a1fb6..d842bf6d71101 100644 --- a/src/libcore/cell.rs +++ b/src/libcore/cell.rs @@ -1103,10 +1103,20 @@ impl<'b> BorrowRef<'b> { fn new(borrow: &'b Cell) -> Option> { let b = borrow.get().wrapping_add(1); if !is_reading(b) { - // If there's currently a writing borrow, or if incrementing the - // refcount would overflow into a writing borrow. + // Incrementing borrow can result in a non-reading value (<= 0) in these cases: + // 1. It was < 0, i.e. there are writing borrows, so we can't allow a read borrow + // due to Rust's reference aliasing rules + // 2. It was isize::max_value() (the max amount of reading borrows) and it overflowed + // into isize::min_value() (the max amount of writing borrows) so we can't allow + // an additional read borrow because isize can't represent so many read borrows + // (this can only happen if you mem::forget more than a small constant amount of + // `Ref`s, which is not good practice) None } else { + // Incrementing borrow can result in a reading value (< 0) in these cases: + // 1. It was = 0, i.e. it wasn't borrowed, and we are taking the first read borrow + // 2. It was > 0 and < isize::max_value(), i.e. there were read borrows, and isize + // is large enough to represent having one more read borrow borrow.set(b); Some(BorrowRef { borrow }) } From 44c165074b1b034799c6e1b95a871cde46755632 Mon Sep 17 00:00:00 2001 From: Luca Barbieri Date: Sun, 21 Jul 2019 12:50:16 +0200 Subject: [PATCH 3/3] fix comment --- src/libcore/cell.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libcore/cell.rs b/src/libcore/cell.rs index d842bf6d71101..f76058302fc14 100644 --- a/src/libcore/cell.rs +++ b/src/libcore/cell.rs @@ -1113,7 +1113,7 @@ impl<'b> BorrowRef<'b> { // `Ref`s, which is not good practice) None } else { - // Incrementing borrow can result in a reading value (< 0) in these cases: + // Incrementing borrow can result in a reading value (> 0) in these cases: // 1. It was = 0, i.e. it wasn't borrowed, and we are taking the first read borrow // 2. It was > 0 and < isize::max_value(), i.e. there were read borrows, and isize // is large enough to represent having one more read borrow