Add Ref/RefMut map_split method #51466
Conversation
|
r? @shepmaster (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
joshlf
force-pushed
the
ref-split
branch
2 times, most recently
from
123f1f1
to
6f065e6
Compare
kennytm
added
the
T-libs-api
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
src/libcore/cell.rs
Outdated
| // | ||
| // `Ref` and `RefMut` are both two words in size, and so there can never be | ||
| // enough `Ref`s or `RefMut`s in existence to overflow half of the `usize` | ||
| // range. Thus, a `BorrowFlag` will never overflow. |
There was a problem hiding this comment.
I doubt this, since you could forget a Ref. See #25841.
There was a problem hiding this comment.
Good call. I'll make sure that this is asserted everywhere it could overflow.
There was a problem hiding this comment.
OK, I believe I've gotten all of the cases.
joshlf
force-pushed
the
ref-split
branch
2 times, most recently
from
3ab1411
to
80bcd16
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
cc @RalfJung on the soundness of this (seems sound to me...) It would be good to list more use cases this supports. let mut borrow = c.borrow_mut();
let (begin, end) = borrow.split_at_mut(2);
assert_eq!(*begin, [1, 2]);
assert_eq!(*end, [3, 4]);
begin.copy_from_slice(&[4, 3]);
end.copy_from_slice(&[2, 1]);which seems more direct. |
src/libcore/cell.rs
Outdated
| /// ``` | ||
| /// use std::cell::{Ref, RefCell}; | ||
| /// | ||
| /// let c = RefCell::new([1, 2, 3, 4]); |
src/libcore/cell.rs
Outdated
| /// ``` | ||
| /// use std::cell::{RefCell, RefMut}; | ||
| /// | ||
| /// let c = RefCell::new([1, 2, 3, 4]); |
kennytm
added
the
T-lang
|
Too deep for me! r? @dtolnay |
|
Why |
src/libcore/cell.rs
Outdated
| // 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::drop `Ref`s or `RefMut`s. Thus, all code must explicitly check for |
There was a problem hiding this comment.
I believe the trouble arises from mem::forget, not mem::drop.
|
The implementation looks good to me. I agree that after split they should be able to have different types |
Done. Is there any canonical way to have the function return an arbitrary number of elements in the tuple (or at least, up to some reasonable bound, like 8)? I have a function called |
src/libcore/cell.rs
Outdated
| /// assert_eq!(*begin, [1, 2]); | ||
| /// assert_eq!(*end, [3, 4]); | ||
| /// ``` | ||
| #[unstable(feature = "refcell_map_split", issue = "51466")] |
There was a problem hiding this comment.
Please file a tracking issue separate from the PR.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
I was thinking of this myself. This is much like the issue faced by Libraries often solve this sort of problem by making a trait for the output type and implementing it for many tuple sizes (or implementing it on a small set of combinator types), but I seldom see this in the standard library... almost as though we're waiting for variadic generics (yet nobody wants to admit it!). |
I'm happy to merge this as is and just keep an eye out for being able to add something like |
|
It does make me wonder though if perhaps the standard library should simply expose a low-level API for this; one which might not be very ergonomic (it might even be (that said, doing so would ironically come at a much greater design cost, as care must be taken not to place unnecessary constraints on future evolution of the type) Edit: Of course, I guess there's no real reason |
|
⌛ Testing commit 2a999b4 with merge 7df0770a97fce4eb8c8c4282ae7f05fe22731328... |
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bors
added
S-waiting-on-bors
Add Ref/RefMut map_split method As proposed [here](https://internals.rust-lang.org/t/make-refcell-support-slice-splitting/7707). TLDR: Add a `map_split` method that allows multiple `RefMut`s to exist simultaneously so long as they refer to non-overlapping regions of the original `RefCell`. This is useful for things like the slice `split_at_mut` method.
|
☀️ Test successful - status-appveyor, status-travis |
|
This PR regressed compile time significantly, as shown by http://perf.rust-lang.org/compare.html?start=0f8f4903f73a21d7f408870551c08acd051abeb0&end=aec00f97e1cdcea2b079e209a7e759201ba6ca7c&stat=instructions:u Almost all benchmarks in rustc-perf were affected, the worst by 4%. @joshlf: Can you investigate? CC @rust-lang/wg-compiler-performance |
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.
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.
Add Ref/RefMut map_split method As proposed [here](https://internals.rust-lang.org/t/make-refcell-support-slice-splitting/7707). TLDR: Add a `map_split` method that allows multiple `RefMut`s to exist simultaneously so long as they refer to non-overlapping regions of the original `RefCell`. This is useful for things like the slice `split_at_mut` method.
DO NOT MERGE: map_split perf test This PR represents the cherry-pick of #51466 and #51630 onto [b81da2](b81da27) (the commit just before #51466). Comparing the performance of this PR to the performance of b81da2 should give us an apples-to-apples comparison of the optimized version of `map_split` against a previous world in which `map_split` doesn't exist, and refcounts can only represent a single writing reference. cc @nnethercote @rkruppe @RalfJung @kennytm
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.
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.
|
@nnethercote 's concern has been addressed in #51630. |
As proposed here.
TLDR: Add a
map_splitmethod that allows multipleRefMuts to exist simultaneously so long as they refer to non-overlapping regions of the originalRefCell. This is useful for things like the slicesplit_at_mutmethod.