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.

Sign up for GitHub

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

Jump to bottom

Add alteration methods to IArray #34

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add alteration methods to IArray #34

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
168 changes: 168 additions & 0 deletions src/array.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,12 @@ impl<T: ImplicitClone + 'static> FromIterator<T> for IArray<T> {
}
}

impl<T: ImplicitClone + 'static> Extend<T> for IArray<T> {
fn extend<I: IntoIterator<Item = T>>(&mut self, iter: I) {
self.insert_many(self.len(), iter);
}
}

impl<T: ImplicitClone + 'static> ImplicitClone for IArray<T> {}

impl<T: ImplicitClone + 'static> From<&'static [T]> for IArray<T> {
Expand Down Expand Up @@ -160,6 +166,161 @@ impl<T: ImplicitClone + 'static> IArray<T> {
Self::Rc(a) => a.get(index).cloned(),
}
}

/// Makes a mutable reference into the array.
///
/// If this array is an `Rc` with no other strong or weak references, returns
/// a mutable slice of the contained data without any cloning. Otherwise, it clones the
/// data into a new array and returns a mutable slice into that.
///
/// # Example
///
/// ```
/// # use implicit_clone::unsync::*;
/// # use std::rc::Rc;
/// // This will reuse the Rc storage
/// let mut v1 = IArray::<u8>::Rc(Rc::new([1,2,3]));
/// v1.make_mut()[1] = 123;
/// assert_eq!(&[1,123,3], v1.as_slice());
///
/// // This will create a new copy
/// let mut v2 = IArray::<u8>::Static(&[1,2,3]);
/// v2.make_mut()[1] = 123;
/// assert_eq!(&[1,123,3], v2.as_slice());
/// ```
#[inline]
pub fn make_mut(&mut self) -> &mut [T] {
// This code is somewhat weirdly written to work around https://github.com/rust-lang/rust/issues/54663 -
// we can't just check if this is an Rc with one reference with get_mut in an if branch and copy otherwise,
// since returning the mutable slice extends its lifetime for the rest of the function.
match self {
Self::Rc(ref mut rc) => {
if Rc::get_mut(rc).is_none() {
*rc = rc.iter().cloned().collect::<Rc<[T]>>();
}
Rc::get_mut(rc).unwrap()
}
Self::Static(slice) => {
*self = Self::Rc(slice.iter().cloned().collect());
match self {
Self::Rc(rc) => Rc::get_mut(rc).unwrap(),
_ => unreachable!(),
}
}
}
}

/// Inserts several objects into the array.
///
/// This overwrites `self` to a new refcounted array with clones of the previous items,
/// with items from the `values` iterator inserted starting at the specified index, shifting
/// later items down.
///
/// # Panics
///
/// Panics if the index is greater than one more than the length of the array.
///
/// # Example
///
/// ```
/// # use implicit_clone::unsync::*;
/// let mut v = IArray::<u8>::Static(&[1,2,6]);
/// v.insert_many(2, [3,4,5]);
/// assert_eq!(&[1,2,3,4,5,6], v.as_slice());
/// ```
pub fn insert_many<I: IntoIterator<Item = T>>(&mut self, index: usize, values: I) {
let head = self.as_slice()[..index].iter().cloned();
let tail = self.as_slice()[index..].iter().cloned();
let rc = head.chain(values).chain(tail).collect();
Copy link
Collaborator

@kirillsemyonkin kirillsemyonkin Oct 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I guess our main complaint in accordance with #17 is this .collect() here allocates a whole new array.

When iterating over items, sure, they can be cloned, since each item iteration makes a clone (ImplicitClone that is, meaning not even allocating anything besides on the stack, just doing things like incrementing an Rc count, for example) and then throws that clone out once its done.

Allocating a new array (also, WASM (since its a yew-stack crate) performance for such allocations I'm not sure about) means looking for a lot of space (both of the arrays combined could be massive - literally, in my native language word "array" translates to "massive"), and then moving all of the made clones into it. This means adding a new single item would require making a whole array clone of a worst-case size. Imagine adding two in a row?

Sure, the user could make some kind of temporary storage to insert all of new items at once, but the API cannot prove that user would not call push for adding single items in a big loop instead. Colloquially in IT and other areas of engineering this is called making the design "idiot-proof". Vec does actually amortize this kind of cost with separating size and capacity. Most of optimizations we could work on (talked over in #17) would probably lead us to just an inferior Vec/Rc<Vec>.

I believe its better to make the user do an intentional to_vec (I wonder if its possible to optimize into a into_vec(self) like make_mut so it just copies bytes (moves?) into newly allocated Vec in case there is only one Rc?) or newly added make_mut for just mutating some items, or get_mut (I already commented that, I'll be waiting for that to be added / explained why it could not be added).

Copy link
Contributor Author

@ColonelThirtyTwo ColonelThirtyTwo Oct 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The only way Rc<[T]> -> Vec<T> could be more efficient than moving out of the Rc<[T]> buffer would be if Vec<T> reused the storage of Rc<[T]>, but it can't - the Rc buffer would be prefixed by the refcount, which Vec won't know about and won't deallocate correctly. You can't even move values out of an Rc<[T]> because try_unwrap and into_inner need [T]: Sized. So copying is really the only way.

to_vec -> edit -> collect::<Rc<[T]>> would involve two copies, which isn't great. Using iterators over the original slice would be better but gets complex.

IMO the simple insert/remove methods are convenient for the usual UI things of adding/removing a single element, though I agree they are kinda footguns for more involved usage.

Copy link
Collaborator

@kirillsemyonkin kirillsemyonkin Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The only way Rc<[T]> -> Vec<T> could be more efficient than moving out of the Rc<[T]> buffer would be if Vec<T> reused the storage of Rc<[T]>

The thought is that we don't necessarily clone each item into the new Vec, but move them instead. In most cases, a move is just like a copy, if not precisely that (but doing it manually would require unsafe and working with Pin I guess (also, do we need to handle Pin<IArray/IString/etc> somehow?)).

You can't even move values out of an Rc<[T]> because try_unwrap and into_inner need [T]: Sized. So copying is really the only way.

I guess this is our limiting factor. Probably making a tracking issue on this repo that would ask to add to Rust into_vec or something of the sort to the Rc<[...]>, unless using unsafe in this repo is a non-issue.

Copy link
Contributor Author

@ColonelThirtyTwo ColonelThirtyTwo Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Even with unsafe, you'd have to somehow be able to free the underlying Rc allocation without dropping the items, and unlike with Vec, there's no guarantees on the allocation that Rc makes, since there's a refcount on it as well. Rust would need to add something like Rc::drain.

EDIT: now that I think about it, you may be able to do it by transmuting to Rc<[MaybeUninit<T>]>, moving the values out, then dropping the transmuted Rc, which will free the memory without running destructors. Assuming that's the only reference, of course.

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't mind that this go in a separate PR so we can merge this one already.

*self = Self::Rc(rc);
}

/// Inserts an object into the array.
///
/// This overwrites `self` to a new refcounted array with clones of the previous items,
/// with `value` inserted at the `index`, shifting later items down.
///
/// [`Self::insert_many`] will be more efficient if inserting multiple items.
///
/// # Panics
///
/// Panics if the index is greater than one more than the length of the array.
///
/// # Example
///
/// ```
/// # use implicit_clone::unsync::*;
/// let mut v = IArray::<u8>::Static(&[1,2,4]);
/// v.insert(2, 3);
/// assert_eq!(&[1,2,3,4], v.as_slice());
/// ```
pub fn insert(&mut self, index: usize, value: T) {
self.insert_many(index, std::iter::once(value));
}

/// Adds an object to the end of the array.
///
/// This overwrites `self` to a new refcounted array with clones of the previous items,
/// with `value` added at the end.
///
/// [`Self::extend`] will be more efficient if inserting multiple items.
///
/// # Example
///
/// ```
/// # use implicit_clone::unsync::*;
/// let mut v = IArray::<u8>::Static(&[1,2,3]);
/// v.push(4);
/// assert_eq!(&[1,2,3,4], v.as_slice());
/// ```
pub fn push(&mut self, value: T) {
self.insert(self.len(), value);
}

/// Removes a range of items from the array.
///
/// This overwrites `self` to a new refcounted array with clones of the previous items, excluding
/// the items covered by `range`, with later items shifted up.
///
/// # Panics
///
/// Panics if the range is out of bounds.
///
/// # Example
///
/// ```
/// # use implicit_clone::unsync::*;
/// let mut v = IArray::<u8>::Static(&[1,2,10,20,3]);
/// v.remove_range(2..4);
/// assert_eq!(&[1,2,3], v.as_slice());
/// ```
pub fn remove_range(&mut self, range: std::ops::Range<usize>) {
let head = self.as_slice()[..range.start].iter().cloned();
let tail = self.as_slice()[range.end..].iter().cloned();
let rc = head.chain(tail).collect();
*self = Self::Rc(rc);
}

/// Removes an item from the array.
///
/// This overwrites `self` to a new refcounted array with clones of the previous items, excluding
/// the items at `index`, with later items shifted up.
///
/// # Panics
///
/// Panics if the index is out of bounds.
///
/// # Example
///
/// ```
/// # use implicit_clone::unsync::*;
/// let mut v = IArray::<u8>::Static(&[1,2,10,3]);
/// v.remove(2);
/// assert_eq!(&[1,2,3], v.as_slice());
/// ```
pub fn remove(&mut self, index: usize) {
self.remove_range(index..index + 1)
}
}

impl<'a, T, U, const N: usize> PartialEq<&'a [U; N]> for IArray<T>
Expand Down Expand Up @@ -276,4 +437,11 @@ mod test_array {
const _ARRAY_F32: IArray<f32> = IArray::Static(&[]);
const _ARRAY_F64: IArray<f64> = IArray::Static(&[]);
}

#[test]
fn extend() {
let mut array = [1, 2, 3].into_iter().collect::<IArray<u32>>();
array.extend([4, 5, 6]);
assert_eq!(&[1, 2, 3, 4, 5, 6], array.as_slice());
}
}
Loading