Skip to content

std: Optimize Vec::from_iter #22200

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

Merged
merged 1 commit into from
Feb 13, 2015
Merged

std: Optimize Vec::from_iter #22200

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
12 changes: 11 additions & 1 deletion src/libcollections/vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1380,7 +1380,17 @@ impl<T> FromIterator<T> for Vec<T> {
fn from_iter<I:Iterator<Item=T>>(iterator: I) -> Vec<T> {
let (lower, _) = iterator.size_hint();
let mut vector = Vec::with_capacity(lower);
for element in iterator {

let mut i = iterator.fuse();
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps obvious to you superbrains but it took me a while to puzzle out why the fuse was necessary here -- maybe worth a comment?

If I understand correctly, the problem is just that the first loop may exhaust the iterator, in which case the second loop would potentially re-run some iterators.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, I'll open a new PR to add a comment, your understanding is spot on.

for element in i.by_ref().take(vector.capacity()) {
let len = vector.len();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious, why do you re-load the len every iteration? I would think we know exactly what the length will be.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since len is just a field, this code is basically just read/writing to the Vec's len field as a local variable, without the need for an actual local variable. (and setting the len in the loop is important for guarding against panics)

Copy link
Contributor

Choose a reason for hiding this comment

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

Setting the len is definitely important, I just wasn't clear on why loading it was necessary / a good idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah re-loading isn't required or necessary, we could keep a separate variable, this was just a pattern used elsewhere in vec.rs so I figured I'd do the same.

unsafe {
ptr::write(vector.get_unchecked_mut(len), element);
vector.set_len(len + 1);
}
}

for element in i {
vector.push(element)
}
vector
Expand Down