Implement TrustedLen for Take<Repeat> and Take<RangeFrom> #47944

oberien · 2018-02-01T20:31:32Z

This will allow optimization of simple repeat(x).take(n).collect() iterators, which are currently not vectorized and have capacity checks.

This will only support a few aggregates on Repeat and RangeFrom, which might be enough for simple cases, but doesn't optimize more complex ones. Namely, Cycle, StepBy, Filter, FilterMap, Peekable, SkipWhile, Skip, FlatMap, Fuse and Inspect are not marked TrustedLen when the inner iterator is infinite.

Previous discussion can be found in #47082

r? @alexcrichton

rust-highfive · 2018-02-01T20:31:44Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

bluss · 2018-02-01T20:42:48Z

src/libcore/iter/range.rs

@@ -325,6 +325,9 @@ impl<A: Step> Iterator for ops::RangeFrom<A> {
 #[unstable(feature = "fused", issue = "35602")]
 impl<A: Step> FusedIterator for ops::RangeFrom<A> {}

+#[unstable(feature = "trusted_len", issue = "37572")]
+unsafe impl<A: Step> TrustedLen for ops::RangeFrom<A> {}
+


Uh oh, this gets into the discussion about the "length of a x.. range" again.. I suppose it fits, it can panic before reaching the trusted length just as much as any_iterator.map(f) can.

If it panics, it diverges, which should be fine. Worst case is that e.g. a Vector is currently collecting the elements. This will leave it with uninitialized values, but it should be dropped anyway / it shouldn't be accessable anymore due to the panic.
Edit: Repeat can panic as well if Clone panics.

Linking previous discussion about why RangeFrom needs to have an infinite size hint, which was also about Take<RangeFrom<_>>: #42315 (comment)

@oberien Yes, checking again to make sure, Vec's special case using TrustedLen trusts the length for the reallocation but it also behaves correctly on a possible panic in Iterator::next().

bluss

Looks good to me. Please add tests for the new code paths.

oberien · 2018-02-01T20:49:27Z

I can't really test that the assembler will be optimized, can I? So I'll just add tests for the size_hint specialization of Take<TrustedLen>.

alexcrichton · 2018-02-01T20:56:05Z

r? @bluss

kennytm · 2018-02-01T20:56:08Z

@oberien In fact you can. You can write a codegen test.

bluss · 2018-02-01T21:19:06Z

A test that exercises the new code path at all would be good, something that checks it produces the expected result. For example using Vec::from_iter(with an affected iterator).

bluss · 2018-02-01T21:22:30Z

Anything TrustedLen is critical for safety so second reviews from contributors are very welcome, just ask if something is unclear. Or suggest something to be clarified in TrustedLen's documented contract.

scottmcm · 2018-02-02T07:42:04Z

src/libcore/iter/mod.rs

-        };
-
-        (lower, upper)
+        TakeImpl::size_hint(self)


Is the specialization actually needed here? LLVM already collapses repeat(x).take(n).size_hint().0 => n today (play demo), since Repeat::size_hint is a constant, so I'm not convinced the duplicate size_hint implementation is needed—just the TrustedLen marker impls ought to be plenty.

The specialization is needed to handle the case of inner.size_hint() returning (x, None) with x being smaller than self.n. With the non-specialized implementation this would result in Take::size_hint returning (x, Some(self.n)), which breaks the contract of TrustedLen that the lower and upper bound must be equal if the upper bound is not None.

Is that allowed? I thought TrustedLen could only have size_hints of (x, Some(x)) or (usize::MAX, None), since that's the most correct lower bound for "if the actual iterator length is larger than usize::MAX".

@scottmcm Does the impl here break the contract? Please show an example if that's the case.

The trait definition implies that the lower bound is irrelevant ( https://doc.rust-lang.org/std/iter/trait.TrustedLen.html )

The iterator reports a size hint where it is either exact (lower bound is equal to upper bound), or the upper bound is None. The upper bound must only be None if the actual iterator length is larger than usize::MAX.

If I recall correctly checks regarding this in libcore are only checking for the lower bound if there is an upper bound.

EDIT: For example SpecExtend for Vec only has a check for the upper bound. They only have a debug_assert! to check that the upper bound is equal to the lower bound if the upper bound is Some.

@bluss I updated the documentation of TrustedLen and removed the now unnecessary specialization of Take<TrustedLen>::size_hint. Could you please double-check that all iterators implementing TrustedLen actually follow this rule? The only one I found is Range<{u,i}32> if we're on a 16bit platform (i.e. 8bit, which should be negligible). I don't know if Rust is supporting any 16bit platform.

Rust supports 16-bit

I opened #48006 for this case.

Survey of TrustedLen:

Simple: Empty (0,Some(0)); Once -> Option::IntoIter -> Item; the other two Option iterators also go to Item; the result iterators are like the option ones and are just 1 or 0; slice iterators are always finite by definition (they just return (.len(), Some(.len()))); vec IntoIter is also already bounded by usize

Forwards to inner: Rev, Cloned, Map, Enumerate, str::Bytes (to cloned slice iter), &mut I

Interesting: Chain makes things longer, and the addition is correct; Zip, as discussed, already required .0 == .1.unwrap_or(usize::MAX) for correctness

Supporting chain was in the design of TrustedLen 😄

oberien · 2018-02-02T08:58:11Z

I think we should add to the documentation of TrustedLen that the iterator is also allowed to diverge. Currently it states

The iterator must produce exactly the number of elements it reported.

I think we should change it to

The iterator must produce exactly the number of elements it reported or diverge.

What do you think?

oberien · 2018-02-02T10:22:52Z

src/test/codegen/repeat-trusted-len.rs

+#[no_mangle]
+pub fn range_from_take_collect() -> Vec<u8> {
+// CHECK: %broadcast.splatinsert = insertelement <{{[0-9]+}} x i8> undef, i8 %{{.*}}, i32 0
+// CHECK: %broadcast.splat = shufflevector <[[WIDTH:[0-9]+]] x i8> %broadcast.splatinsert, <[[WIDTH]] x i8> undef, <[[WIDTH]] x i32> zeroinitializer


Should I use CHECK-NEXT here instead?

I don't know the details here. Using CHECK-NEXT where possible seems very good. Are these the right instructions to match on? If it's too fragile or not meaningfully testing codegen it might be better to skip this function.

This test might be a bit fragile, but I don't know how else I'd test for llvm vectorizing the operations. In this test I'm checking that llvm extends the last written number into a vector, which should indicate with high certainty that it'll perform vectorized operations.

bluss · 2018-02-02T16:49:04Z

@oberien I think mentioning that the iterator can also panic before reaching the end is best, it is more clear. Just to avoid the confusion with an infinite iterator (which is not ok if a finite number of elements was promised.)

bluss · 2018-02-02T16:50:49Z

src/test/codegen/repeat-trusted-len.rs

+#[no_mangle]
+pub fn repeat_take_collect() -> Vec<u8> {
+// CHECK: call void @llvm.memset.p0i8
+    iter::repeat(42).take(100000).collect()


bluss · 2018-02-02T17:30:31Z

Thanks, this is a very nice improvement!

@bors r+

bors · 2018-02-02T17:30:32Z

📌 Commit ee8b4ca has been approved by bluss

@alexcrichton

…n, r=bluss Implement TrustedLen for Take<Repeat> and Take<RangeFrom> This will allow optimization of simple `repeat(x).take(n).collect()` iterators, which are currently not vectorized and have capacity checks. This will only support a few aggregates on `Repeat` and `RangeFrom`, which might be enough for simple cases, but doesn't optimize more complex ones. Namely, Cycle, StepBy, Filter, FilterMap, Peekable, SkipWhile, Skip, FlatMap, Fuse and Inspect are not marked `TrustedLen` when the inner iterator is infinite. Previous discussion can be found in rust-lang#47082 r? @alexcrichton

Rollup of 9 pull requests - Successful merges: #47753, #47862, #47877, #47896, #47912, #47944, #47947, #47978, #47958 - Failed merges:

kennytm · 2018-02-03T22:55:03Z

@bors r-

The new codegen test failed in i686-musl.

[01:02:09] failures:
[01:02:09] 
[01:02:09] ---- [codegen] codegen/repeat-trusted-len.rs stdout ----
[01:02:09] 	
[01:02:09] error: verification with 'FileCheck' failed
[01:02:09] status: exit code: 1
[01:02:09] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/llvm/build/bin/FileCheck" "--input-file" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/repeat-trusted-len.ll" "/checkout/src/test/codegen/repeat-trusted-len.rs"
[01:02:09] stdout:
[01:02:09] ------------------------------------------
[01:02:09] 
[01:02:09] ------------------------------------------
[01:02:09] stderr:
[01:02:09] ------------------------------------------
[01:02:09] /checkout/src/test/codegen/repeat-trusted-len.rs:28:11: error: expected string not found in input
[01:02:09] // CHECK: %[[SPLATINSERT:.*]] = insertelement <{{[0-9]+}} x i8> undef, i8 %{{.*}}, i32 0
[01:02:09]           ^
[01:02:09] /checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/repeat-trusted-len.ll:171:37: note: scanning from here
[01:02:09] define void @range_from_take_collect(%"alloc::vec::Vec<u8>"* noalias nocapture sret dereferenceable(12)) unnamed_addr #0 personality i32 (i32, i32, i64, %"unwind::libunwind::_Unwind_Exception"*, %"unwind::libunwind::_Unwind_Context"*)* @rust_eh_personality {
[01:02:09]                                     ^
[01:02:09] /checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/repeat-trusted-len.ll:197:2: note: possible intended match here
[01:02:09]  %9 = getelementptr inbounds [0 x i8], [0 x i8]* %7, i32 0, i32 %8
[01:02:09]  ^
[01:02:09] 
[01:02:09] ------------------------------------------

oberien · 2018-02-03T23:44:28Z

I guess I'm just going to delete that test then. It doesn't seem to be sophisticated enough after all.

bluss · 2018-02-04T09:57:54Z

@bors r+

bors · 2018-02-04T09:57:55Z

📌 Commit 6edd22e has been approved by bluss

bluss · 2018-02-04T14:49:39Z

@oberien Do you want to rebase (and squash some commits) this PR?

oberien · 2018-02-04T15:30:08Z

@bluss Done. Also a notifier for #47944 (comment) in case you missed that one (comment on outdated diff)

bluss · 2018-02-04T20:00:14Z

Thanks. I'll have to come back to do the review of those impls a different evening.

scottmcm · 2018-02-04T23:20:08Z

src/libcore/iter/traits.rs

@@ -970,9 +970,11 @@ impl<'a, I: FusedIterator + ?Sized> FusedIterator for &'a mut I {}
 /// The iterator reports a size hint where it is either exact
 /// (lower bound is equal to upper bound), or the upper bound is [`None`].
 /// The upper bound must only be [`None`] if the actual iterator length is
-/// larger than [`usize::MAX`].
+/// larger than [`usize::MAX`]. In that case, the lower bound must be
+/// [`usize::MAX`], resulting in a [`.size_hint`] of `(usize::MAX, None)`.


👍 to this. Probably worth mentioning in release notes just in case, though?

How would I go about doing so? I don't know if it's really needed because TrustedLen is unstable anyways. But better safe than sorry.

I think for now it just means someone with tagging permissions (thus not me, at least) putting relnotes on this.

It doesn't have an impact to users of stable Rust, so then it's normally not in any release notes.

Ah, makes sense. I guess nightly finds out by watching the TWiR PRs section.

bluss · 2018-02-06T19:45:33Z

Nice collaboration here. Since the identified bug is orthogonal, I think we should just let this merge.

@bors r+

bors · 2018-02-06T19:45:34Z

📌 Commit 6caec2c has been approved by bluss

@alexcrichton

…n, r=bluss Implement TrustedLen for Take<Repeat> and Take<RangeFrom> This will allow optimization of simple `repeat(x).take(n).collect()` iterators, which are currently not vectorized and have capacity checks. This will only support a few aggregates on `Repeat` and `RangeFrom`, which might be enough for simple cases, but doesn't optimize more complex ones. Namely, Cycle, StepBy, Filter, FilterMap, Peekable, SkipWhile, Skip, FlatMap, Fuse and Inspect are not marked `TrustedLen` when the inner iterator is infinite. Previous discussion can be found in rust-lang#47082 r? @alexcrichton

Rollup of 10 pull requests - Successful merges: #47613, #47631, #47810, #47883, #47922, #47944, #48014, #48018, #48020, #48028 - Failed merges:

rust-highfive assigned alexcrichton Feb 1, 2018

oberien mentioned this pull request Feb 1, 2018

Add UnboundedIterator Trait #47082

Closed

bluss reviewed Feb 1, 2018

View reviewed changes

kennytm added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 1, 2018

rust-highfive assigned bluss and unassigned alexcrichton Feb 1, 2018

scottmcm reviewed Feb 2, 2018

View reviewed changes

oberien force-pushed the unboundediterator-trustedlen branch from 806002e to 45e63ae Compare February 2, 2018 10:02

oberien commented Feb 2, 2018

View reviewed changes

bluss reviewed Feb 2, 2018

View reviewed changes

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 3, 2018

kennytm mentioned this pull request Feb 3, 2018

Rollup of 9 pull requests #47984

Closed

bors added a commit that referenced this pull request Feb 3, 2018

Auto merge of #47984 - kennytm:rollup, r=kennytm

6f6ac5e

Rollup of 9 pull requests - Successful merges: #47753, #47862, #47877, #47896, #47912, #47944, #47947, #47978, #47958 - Failed merges:

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 4, 2018

oberien added 3 commits February 4, 2018 16:09

Implement TrustedLen for Take<Repeat> and Take<RangeFrom>

a1809d5

TrustedLen for Repeat / RangeFrom test cases

75474ff

Document TrustedLen guarantees more explicitly

6caec2c

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 4, 2018

oberien force-pushed the unboundediterator-trustedlen branch from 3ec20f0 to 6caec2c Compare February 4, 2018 15:29

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 4, 2018

oberien mentioned this pull request Feb 4, 2018

Memory Unsafety on 16bit Platforms for Range.collect() #48006

Closed

scottmcm reviewed Feb 4, 2018

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 6, 2018

Manishearth mentioned this pull request Feb 7, 2018

Rollup of 10 pull requests #48053

Merged

bors added a commit that referenced this pull request Feb 7, 2018

Auto merge of #48053 - Manishearth:rollup, r=Manishearth

29c8276

Rollup of 10 pull requests - Successful merges: #47613, #47631, #47810, #47883, #47922, #47944, #48014, #48018, #48020, #48028 - Failed merges:

bors merged commit 6caec2c into rust-lang:master Feb 7, 2018

oberien mentioned this pull request Feb 17, 2018

Collect<Vec<u16>> from range doesn't optimize well. #43124

Closed

Implement TrustedLen for Take<Repeat> and Take<RangeFrom> #47944

Implement TrustedLen for Take<Repeat> and Take<RangeFrom> #47944

Conversation

oberien commented Feb 1, 2018

rust-highfive commented Feb 1, 2018

Choose a reason for hiding this comment

oberien Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluss left a comment

Choose a reason for hiding this comment

oberien commented Feb 1, 2018

alexcrichton commented Feb 1, 2018

kennytm commented Feb 1, 2018

bluss commented Feb 1, 2018

bluss commented Feb 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oberien Feb 3, 2018 • edited Loading

Choose a reason for hiding this comment

oberien Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oberien commented Feb 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluss commented Feb 2, 2018

Choose a reason for hiding this comment

bluss commented Feb 2, 2018

bors commented Feb 2, 2018

kennytm commented Feb 3, 2018

oberien commented Feb 3, 2018

bluss commented Feb 4, 2018

bors commented Feb 4, 2018

bluss commented Feb 4, 2018 • edited Loading

oberien commented Feb 4, 2018

bluss commented Feb 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluss commented Feb 6, 2018

bors commented Feb 6, 2018

oberien Feb 1, 2018 •

edited

Loading

oberien Feb 3, 2018 •

edited

Loading

oberien Feb 4, 2018 •

edited

Loading

bluss commented Feb 4, 2018 •

edited

Loading