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

Add back Vec::from_elem #832

Merged
merged 2 commits into from
Feb 16, 2015
Merged
Changes from 1 commit
Commits
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
73 changes: 73 additions & 0 deletions text/0000-from-elem-with-love.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
- Feature Name: from_elem_with_love
- Start Date: 2015-02-11
- RFC PR: (leave this empty)
- Rust Issue: (leave this empty)

# Summary

Add back `Vec::from_elem`.

# Motivation

High demand, mostly. There are currently a few ways to achieve the behaviour of `Vec::from_elem(elem, n)`:

```
// #1
let vec = Vec::new();
for i in range(0, n) {
vec.push(elem.clone())
}
```

```
// #2
let vec = vec![elem; n]
```

```
// #3
let vec = Vec::new();
vec.resize(elem, n);
```

```
// #4
let vec: Vec<_> = (0..n).map(|_| elem.clone()).collect()
```

```
// #5
let vec: Vec<_> = iter::repeat(elem).take(n).collect();
```

None of these quite match the convenience, power, and performance of:

```
let vec = Vec::from_elem(elem, n)
```

* `#1` is verbose *and* slow, because each `push` requires a capacity check
* `#2` only works for a Copy `elem` and const `n`
* `#3` needs a temporary, but should be otherwise identical performance-wise.
* `#4` and `#5` suffer from the untrusted iterator len problem performance wise (which is only a temporary
argument, this will be solved sooner rather than later), and are otherwise verbose and noisy. They also
Copy link
Member

Choose a reason for hiding this comment

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

To me personally the 5th option is indeed the idiomatic method by which I would recommend creating a Vec today. Opinions of verboseness/noisiness aside, it would be useful to quantify precisely why this this is slow beyond the trusted iterator length problem, as well as backing this claim up with numbers.

I've created a sample benchmark which today resolves the performance argument for options 4/5 (at least for this one use case). This is boiling down to me trying to say that performance is a fixable problem with the exact APIs we have today (especially with trusted iterator lengths) and it may not be a strong enough motivation for this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was basing these claims based on these results I found a month ago.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the link! They're quite helpful.

I removed the from_elem benchmarks (as it was removed) and got the following results with today's compiler plus the modified version from above: https://gist.github.com/alexcrichton/1b17e3ace982c095a49d. The results are pretty surprising to me and may indicate that an LLVM update has had a regression recently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My laptop was acting super inconsistent when I ran the benches, so I wouldn't be surprised if my results were erroneous. Although iirc Unsafe consistently beat FromIter...

I'd also warn that your trusted_len isn't panic safe (not clear if trusted_len should imply no_panic). disregard. See rust-lang/rust#21465 for detailed discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated my old bench to master, stealing Vec::from_elem from 0.11: https://gist.github.com/Gankro/f40d87b75ae91562892f

Same basic results you got: my trusted impl is busted!

Copy link
Contributor

Choose a reason for hiding this comment

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

That isn't totally sound: Why is the unsafe path slower than the safe path?

need to clone one more time than other methods *strictly* need to.

# Detailed design

Just revert the code deletion.
Copy link
Member

Choose a reason for hiding this comment

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

If we are re-adding from_elem we may wish to reconsider the name of this function. For example there are no other mentions of elem in the Vec API and all mentions of elem in the slice API are unstable. Another possibility could possibly be Vec::repeat.


# Drawbacks

Trivial API bloat, more ways to do the same thing.

Copy link
Member

Choose a reason for hiding this comment

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

Another drawback you may wish to add is inconsistency in APIs. This doesn't also include other list-like collections such as DList, Bitv, or RingBuf.

# Alternatives

Make the `vec![elem; n]` form more powerful. There's literally no reason the author is aware of
for the restrictions it has. It would be shorter. It would be cooler. It would give less ways
Copy link
Member

Choose a reason for hiding this comment

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

This macro currently expands to Box::new([a; b]) which gives us a few pitfalls:

  • Due to box syntax being feature gated, it has to evaluate into a temporary and then store into the destination whereas Vec::from_elem would store directly into the destination.
  • Due to using [a; b] it is subject to compiler restrictions about a and b, hence where the const-ness of b and copy-ness of a is required (no way to codegen otherwise).

If Vec::from_elem is added, we could consider moving this macro to calling that API instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the internals of Vec are exposed, the macro could easily be the implementation of from_elem.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that would fit best actually (the vec![elem; count] form).

Copy link
Member

Choose a reason for hiding this comment

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

I would also love to see this happen. One downside is discoverability (since it's not listed as an API per se), but I think we can emphasize it in the docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a much better solution than adding back Vec::from_elem to me. I wouldn’t want to have to choose between Vec::from_elem and vec![elem; n], and the latter syntax is just much nicer and matches the array syntax anyway.

We could document vec![elem; n] in the same place we document [elem; n] because they’re basically the same thing. (Of course, the differences would have to be emphasised.)

to do the same thing. Also it better clarifies the subtle amiguity of what `Vec::from_elem(100, 10)`
produces. 100 10's? 10 100's?

# Unresolved questions

No.