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

Tracking issue for push_all stabilization #27744

Closed
aturon opened this issue Aug 12, 2015 · 18 comments · Fixed by #30187
Closed

Tracking issue for push_all stabilization #27744

aturon opened this issue Aug 12, 2015 · 18 comments · Fixed by #30187
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@aturon
Copy link
Member

aturon commented Aug 12, 2015

The Vec::push_all method special-cases pushing a slice onto a vector, and historically has performed much better than using extend (depending on how LLVM optimizations are working out).

We need to offer a method with maximal performance somehow, but ideally this would come from extend, either through optimization or through impl specialization.
@aturon aturon added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. labels Aug 12, 2015
@jonas-schievink
Copy link
Contributor

cc #26902

@Gankra
Copy link
Contributor

Gankra commented Aug 12, 2015

Unless specialization lands soon, I'm tempted to just stabilize this as "this will be deprecated when it's no longer necessary".

@Ms2ger Ms2ger mentioned this issue Aug 16, 2015
Closed
99 tasks
@jnordwick
Copy link

Have the IR (lack of) hinting issues been worked out?

@sfackler
Copy link
Member

sfackler commented Nov 4, 2015

Nominating for 1.6 discussion.

@alexcrichton alexcrichton added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed I-nominated labels Nov 5, 2015
@alexcrichton
Copy link
Member

🔔 This issue is now entering its cycle-long final comment period for stabilization 🔔

The conclusion of the libs team is that we can stabilize this and just deprecate it once specialization lands and makes this function obsolete.

@cuviper
Copy link
Member

cuviper commented Nov 5, 2015

IMO, planned deprecation defeats the point of stabilizing it in the first place. i.e. I still won't want to use it if it's definitely going to be yanked out later...

@sfackler
Copy link
Member

sfackler commented Nov 5, 2015

It's not going to be yanked out ever if stabilized. It will eventually be deprecated in favor of extend, but remain in place.

@cuviper
Copy link
Member

cuviper commented Nov 5, 2015

Ok, I thought deprecation was more aggressive than that. No problem then.

@briansmith
Copy link
Contributor

IMO, planned deprecation defeats the point of stabilizing it in the first place. i.e. I still won't want to use it if it's definitely going to be yanked out later...

I agree with this.

It's not going to be yanked out ever if stabilized. It will eventually be deprecated in favor of extend, but remain in place.

IMO, that would be a poor choice.

In the discussion on clone_from_slice, there seemed to be a lot of agreement that the compiler could hard-code some specializations for features that are urgently needed so that they're not blocked on generalized specialization. This seems like a candidate for that kind of treatment.

One of the main reasons for moving ahead with clone_from_slice is that clone_from_slice can be changed to have specific performance guarantees. push_all could be like that, but currently it is not. Unless push_all is improved to have similar performance guarantees, I don't think it should be stabilized.

Conversely, if useful performance guarantees are added to it (at most one resizing of the array, at most one bounds check on each array, equivalent to set_len followed by copy_nonoverlapping for POD/Copy types), then I do think it makes sense to stabilize it since it seems like it would be a long time before extend would be able to guarantee those things.

It is also unfortunate that the name push_all is so different from clone_from_slice. It might be good to rename it push_from_slice, or a different name if clone_from_slice gets renamed.

@erickt
Copy link
Contributor

erickt commented Nov 5, 2015

Would it be worth considering another name for this method? I find push_all slightly ambiguous with respect to extend. Perhaps push_slice would be a bit more clear? It'd blend a bit better with clone_from_slice.

@briansmith briansmith mentioned this issue Nov 6, 2015
Closed
@briansmith
Copy link
Contributor

I propose renaming this function to extend_from_slice, if it is going to be stabilized at all. See #27750 (comment) for the rationale.

@aturon
Copy link
Member Author

aturon commented Nov 9, 2015

@briansmith

In the discussion on clone_from_slice, there seemed to be a lot of agreement that the compiler could hard-code some specializations for features that are urgently needed so that they're not blocked on generalized specialization. This seems like a candidate for that kind of treatment.

I don't think there's widespread agreement about adding ad hoc specializations/intrinsics, barring very exceptional circumstances. But @rust-lang/lang and @rust-lang/compiler may want to weigh in.

One of the main reasons for moving ahead with clone_from_slice is that clone_from_slice can be changed to have specific performance guarantees. push_all could be like that, but currently it is not. Unless push_all is improved to have similar performance guarantees, I don't think it should be stabilized.

Conversely, if useful performance guarantees are added to it (at most one resizing of the array, at most one bounds check on each array, equivalent to set_len followed by copy_nonoverlapping for POD/Copy types), then I do think it makes sense to stabilize it since it seems like it would be a long time before extend would be able to guarantee those things.

I'm a little confused about the overall thrust here. To be clear, the reason that push_all hasn't been stabilized in the past has not been a desire to improve its implementation, but rather to specialize extend to use the push_all implementation. In other words, the point of dissatisfaction was having semantically identical APIs where the client is forced to explicitly call the more specialized one for the best performance.

Now, that said, i think the performance guarantees you're raising are an important (though orthogonal) point. One uncomfortable thing about the current push_all implementation is that it's carefully tailored to trigger the right LLVM optimizations. Of course, it'd be much nicer if we could provide an actual guarantee about the generated code instead -- and not just for this function, but for a handful of others throughout std.

One way to address this desire might be to add more ad hoc intrinsics, but I wonder if there's any way we might eventually allow "inline MIR", so to speak, and whether that'd be enough to express the precise code you want to generate.

On the whole, I still think it's reasonable to stabilize something push_all as the best way we currently have of appending a slice onto a vector. I'm not particularly bothered by the intent to deprecate, both because it will be some time before we're in a position to do so, and because the whole point of the stabilization and deprecation process is to allow relatively smooth evolution without breakage (since deprecated items are not actually removed). We shouldn't let the perfect be the enemy of the good, especially when we have a good process for iterating toward perfection over time :)

It is also unfortunate that the name push_all is so different from clone_from_slice. It might be good to rename it push_from_slice, or a different name if clone_from_slice gets renamed.

@alexcrichton didn't mention this, but no one was particularly a fan of the name. 👍 on renaming, and matching the clone_from_slice idiom probably makes sense.

@Gankra
Copy link
Contributor

Gankra commented Nov 9, 2015

👍 unhappy_no_specialization_extend_slice_effecient

@nrc
Copy link
Member

nrc commented Nov 9, 2015

I don't think there's widespread agreement about adding ad hoc specializations/intrinsics

+1000 to not adding an ad hoc hack here.

However, I'd love to see some kind of push_all stabilised - it's probably the unstable function I use the most.

@bluss
Copy link
Member

bluss commented Nov 9, 2015

I don't think there's widespread agreement about adding ad hoc specializations/intrinsics, barring very exceptional circumstances.

Absolutely, I would say the same thing: it's very unlikely there is any agreement about it. It's just my crazy idea. 😄

@nikomatsakis
Copy link
Contributor

On Mon, Nov 09, 2015 at 01:04:45PM -0800, Nick Cameron wrote:

I don't think there's widespread agreement about adding ad hoc specializations/intrinsics

+1000 to not adding an ad hoc hack here.

and +1K more

@bluss
Copy link
Member

bluss commented Nov 16, 2015

Notice that the std::io::Write's write method for Vec<u8> is already a stable way to get hold of push_all when it comes to u8 elements, arguably the most important case.

@alexcrichton
Copy link
Member

The libs team discussed this during triage today and the decision was to stabilize while renaming the method to extend_from_slice.

@alexcrichton alexcrichton mentioned this issue Dec 3, 2015
Merged
alexcrichton added a commit to alexcrichton/rust that referenced this issue Dec 5, 2015
@alexcrichton
std: Stabilize APIs for the 1.6 release
464cdff 
This commit is the standard API stabilization commit for the 1.6 release cycle.
The list of issues and APIs below have all been through their cycle-long FCP and
the libs team decisions are listed below

Stabilized APIs

* `Read::read_exact`
* `ErrorKind::UnexpectedEof` (renamed from `UnexpectedEOF`)
* libcore -- this was a bit of a nuanced stabilization, the crate itself is now
  marked as `#[stable]` and the methods appearing via traits for primitives like
  `char` and `str` are now also marked as stable. Note that the extension traits
  themeselves are marked as unstable as they're imported via the prelude. The
  `try!` macro was also moved from the standard library into libcore to have the
  same interface. Otherwise the functions all have copied stability from the
  standard library now.
* The `#![no_std]` attribute
* `fs::DirBuilder`
* `fs::DirBuilder::new`
* `fs::DirBuilder::recursive`
* `fs::DirBuilder::create`
* `os::unix::fs::DirBuilderExt`
* `os::unix::fs::DirBuilderExt::mode`
* `vec::Drain`
* `vec::Vec::drain`
* `string::Drain`
* `string::String::drain`
* `vec_deque::Drain`
* `vec_deque::VecDeque::drain`
* `collections::hash_map::Drain`
* `collections::hash_map::HashMap::drain`
* `collections::hash_set::Drain`
* `collections::hash_set::HashSet::drain`
* `collections::binary_heap::Drain`
* `collections::binary_heap::BinaryHeap::drain`
* `Vec::extend_from_slice` (renamed from `push_all`)
* `Mutex::get_mut`
* `Mutex::into_inner`
* `RwLock::get_mut`
* `RwLock::into_inner`
* `Iterator::min_by_key` (renamed from `min_by`)
* `Iterator::max_by_key` (renamed from `max_by`)

Deprecated APIs

* `ErrorKind::UnexpectedEOF` (renamed to `UnexpectedEof`)
* `OsString::from_bytes`
* `OsStr::to_cstring`
* `OsStr::to_bytes`
* `fs::walk_dir` and `fs::WalkDir`
* `path::Components::peek`
* `slice::bytes::MutableByteVector`
* `slice::bytes::copy_memory`
* `Vec::push_all` (renamed to `extend_from_slice`)
* `Duration::span`
* `IpAddr`
* `SocketAddr::ip`
* `Read::tee`
* `io::Tee`
* `Write::broadcast`
* `io::Broadcast`
* `Iterator::min_by` (renamed to `min_by_key`)
* `Iterator::max_by` (renamed to `max_by_key`)
* `net::lookup_addr`

New APIs (still unstable)

* `<[T]>::sort_by_key` (added to mirror `min_by_key`)

Closes rust-lang#27585
Closes rust-lang#27704
Closes rust-lang#27707
Closes rust-lang#27710
Closes rust-lang#27711
Closes rust-lang#27727
Closes rust-lang#27740
Closes rust-lang#27744
Closes rust-lang#27799
Closes rust-lang#27801
cc rust-lang#27801 (doesn't close as `Chars` is still unstable)
Closes rust-lang#28968
bors added a commit that referenced this issue Dec 6, 2015
@bors
Auto merge of #30187 - alexcrichton:stabilize-1.6, r=aturon
c4b1638 
This commit is the standard API stabilization commit for the 1.6 release cycle.
The list of issues and APIs below have all been through their cycle-long FCP and
the libs team decisions are listed below

Stabilized APIs

* `Read::read_exact`
* `ErrorKind::UnexpectedEof` (renamed from `UnexpectedEOF`)
* libcore -- this was a bit of a nuanced stabilization, the crate itself is now
  marked as `#[stable]` and the methods appearing via traits for primitives like
  `char` and `str` are now also marked as stable. Note that the extension traits
  themeselves are marked as unstable as they're imported via the prelude. The
  `try!` macro was also moved from the standard library into libcore to have the
  same interface. Otherwise the functions all have copied stability from the
  standard library now.
* `fs::DirBuilder`
* `fs::DirBuilder::new`
* `fs::DirBuilder::recursive`
* `fs::DirBuilder::create`
* `os::unix::fs::DirBuilderExt`
* `os::unix::fs::DirBuilderExt::mode`
* `vec::Drain`
* `vec::Vec::drain`
* `string::Drain`
* `string::String::drain`
* `vec_deque::Drain`
* `vec_deque::VecDeque::drain`
* `collections::hash_map::Drain`
* `collections::hash_map::HashMap::drain`
* `collections::hash_set::Drain`
* `collections::hash_set::HashSet::drain`
* `collections::binary_heap::Drain`
* `collections::binary_heap::BinaryHeap::drain`
* `Vec::extend_from_slice` (renamed from `push_all`)
* `Mutex::get_mut`
* `Mutex::into_inner`
* `RwLock::get_mut`
* `RwLock::into_inner`
* `Iterator::min_by_key` (renamed from `min_by`)
* `Iterator::max_by_key` (renamed from `max_by`)

Deprecated APIs

* `ErrorKind::UnexpectedEOF` (renamed to `UnexpectedEof`)
* `OsString::from_bytes`
* `OsStr::to_cstring`
* `OsStr::to_bytes`
* `fs::walk_dir` and `fs::WalkDir`
* `path::Components::peek`
* `slice::bytes::MutableByteVector`
* `slice::bytes::copy_memory`
* `Vec::push_all` (renamed to `extend_from_slice`)
* `Duration::span`
* `IpAddr`
* `SocketAddr::ip`
* `Read::tee`
* `io::Tee`
* `Write::broadcast`
* `io::Broadcast`
* `Iterator::min_by` (renamed to `min_by_key`)
* `Iterator::max_by` (renamed to `max_by_key`)
* `net::lookup_addr`

New APIs (still unstable)

* `<[T]>::sort_by_key` (added to mirror `min_by_key`)

Closes #27585
Closes #27704
Closes #27707
Closes #27710
Closes #27711
Closes #27727
Closes #27740
Closes #27744
Closes #27799
Closes #27801
cc #27801 (doesn't close as `Chars` is still unstable)
Closes #28968
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

std: Stabilize APIs for the 1.6 release alexcrichton/rust