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.

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

IArray: avoid heap allocation if there is only one element #38

Merged
merged 6 commits into from
Oct 29, 2023

Conversation

cecton
Copy link
Member

@cecton cecton commented Oct 28, 2023

Working on Yew at the moment and I would like to refactor NodeSeq to use
IArray. It already works as is but I thought it would be nice if we could avoid
unnecessary allocations, particularly when there is only one item in the array.

@kirillsemyonkin
Copy link
Collaborator

One item is obvious good optimization, but what if there is an optimal number other than 1 that could be used?

@cecton
Copy link
Member Author

cecton commented Oct 28, 2023

One item is obvious good optimization, but what if there is an optimal number other than 1 that could be used?

It's best to use Rc in that case. The optimization is so that you can clone the IArray by cloning it's single item directly if there is only one. If there are more items, then every items need to be cloned. So I think in that case it's best to use Rc.

@@ -216,7 +216,8 @@ impl<T: ImplicitClone + 'static> IArray<T> {
pub fn get_mut(&mut self) -> Option<&mut [T]> {
match self {
Self::Rc(ref mut rc) => Rc::get_mut(rc),
Self::Static(_) | Self::Single(_) => None,
Self::Static(_) => None,
Self::Single(ref mut a) => Some(a),
Copy link
Collaborator

Choose a reason for hiding this comment

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

change is not reflected in the doc above?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in f80eb14

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think its different semantics from Rc::get_mut, since every clone of single item arrays will now be mutable and different from original, so iarray != iarray.clone()...some mutation via get_mut()... (Rc prevents mutation via get_mut() if there is a clone), but I guess its fine to do that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Leaving this discussion as unresolved as I think it's interesting for the future

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess its fine to do that?

no idea xD tbh I wasn't too much in favor of adding mutable API. I was a bit in a doubt about that

@cecton
Copy link
Member Author

cecton commented Oct 28, 2023

@kirillsemyonkin do you think I should revert eb484ec?

@kirillsemyonkin
Copy link
Collaborator

@kirillsemyonkin do you think I should revert eb484ec?

Vec supports this via vec![1] or Vec::from([1]), both of which are not that hard to type out. We could be adding iarray!, but for this functionality I think it would be a bit painful each time I would be trying to use Vec::from(T) and figure out that unlike IArray, the one for Vec does not exist. Equalizing the suffering, of sorts.

@cecton
Copy link
Member Author

cecton commented Oct 28, 2023

We could be adding iarray!

Actually... xD #15 I was all in favor for it

@cecton cecton merged commit 6ebe2e2 into yewstack:main Oct 29, 2023
12 checks passed
@cecton cecton deleted the avoid-allocation-for-single-elements branch October 29, 2023 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants