Remove data structure specialization for .zip() iterator #36490

bluss · 2016-09-15T08:10:47Z

Go back on half the specialization, the part that changed the Zip
struct's fields themselves depending on the types of the iterators.

Previous PR: #33090

This means that the Zip iterator will always carry two usize fields,
which are sometimes unused. If a whole for loop using a .zip() iterator is
inlined, these are simply removed and have no effect.

The same improvement for Zip of for example slice iterators remain, and
they still optimize well. However, like when the specialization of zip
was merged, the compiler is still very sensistive to the exact context.

For example this code only autovectorizes if the function is used, not
if the code in zip_sum_i32 is inserted inline where it was called:

fn zip_sum_i32(xs: &[i32], ys: &[i32]) -> i32 {
    let mut s = 0;
    for (&x, &y) in xs.iter().zip(ys) {
        s += x * y;
    }
    s
}

fn zipdot_i32_default_zip(b: &mut test::Bencher)
{
    let xs = vec![1; 1024];
    let ys = vec![1; 1024];

    b.iter(|| {
        zip_sum_i32(&xs, &ys)
    })
}

Include a test that checks that Zip<T, U> is covariant w.r.t. T and U.

Fixes #35727

rust-highfive · 2016-09-15T08:11:01Z

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

pnkfelix · 2016-09-15T10:43:28Z

src/libcore/iter/mod.rs

@@ -643,7 +643,8 @@ impl<A, B> FusedIterator for Chain<A, B>
 pub struct Zip<A, B> {
    a: A,
    b: B,
-    spec: <(A, B) as ZipImplData>::Data,
+    index: usize,


could you add a comment here pointing out that these fields are only used for specialized instantiations of A and B (and point to #35727 in the comment for further explanation)

Done with an amend. Thanks

pnkfelix · 2016-09-15T10:46:44Z

(hmm, these new github UI changes run risk of bypassing bors. good thing I don't have authority to merge...)

Go back on half the specialization, the part that changed the Zip struct's fields themselves depending on the types of the iterators. This means that the Zip iterator will always carry two usize fields, which are unused. If a whole for loop using a .zip() iterator is inlined, these are simply removed and have no effect. The same improvement for Zip of for example slice iterators remain, and they still optimize well. However, like when the specialization of zip was merged, the compiler is still very sensistive to the exact context. For example this code only autovectorizes if the function is used, not if the code in zip_sum_i32 is inserted inline it was called: ``` fn zip_sum_i32(xs: &[i32], ys: &[i32]) -> i32 { let mut s = 0; for (&x, &y) in xs.iter().zip(ys) { s += x * y; } s } fn zipdot_i32_default_zip(b: &mut test::Bencher) { let xs = vec![1; 1024]; let ys = vec![1; 1024]; b.iter(|| { zip_sum_i32(&xs, &ys) }) } ``` Include a test that checks that Zip<T, U> is covariant w.r.t. T and U.

nagisa · 2016-09-15T12:36:01Z

src/test/run-pass/variance-iterators-in-libcore.rs

@@ -0,0 +1,17 @@
+// Copyright 2015 The Rust Project Developers. See the COPYRIGHT


NIT: Name for this test is weirdly general for what it tests. Linked list iterator covariance is tested in libcollections itself for example. Perhaps we could test for zip covariance in libcoretest too?

It's a place to put more iterator tests, for now just this one.

@bluss my experience suggests that people rarely reuse/update tests and instead just add new ones. The only exception I can think of is when some area belongs to one person and they do all the work in it.

I think it has happened before in the various run-pass/sync-send-*.rs files

nagisa · 2016-09-15T12:38:02Z

@pnkfelix the only thing that actually merges anything is clicking the “merge pull request” button AFAICT.

bluss · 2016-09-15T13:47:13Z

We need to get to grips with the iterator optimization stumbling blocks later. What I understand it's still due to spurious null checks being inserted.

The iterator element is Option<(&T, &T)>, so a null in the first half is equal to None and the end of the iteration. It seems even when the expression is explicitly Some(a, b), the null check is even inserted there, which is nonsensical from a Rust perspective -- Some(a, b) is never None.

bluss · 2016-09-15T13:51:13Z

Another sidenote, it's really true that iterators of non-references don't have that problem. a.iter().cloned().zip(b.iter().cloned()) has no problem if a, b are slices -- and if the specialization trait is added on Clone, oops.

brson · 2016-09-16T00:30:58Z

Nominated since it's a regression fix. Might be better to get it out faster.

alexcrichton · 2016-09-16T08:17:07Z

@bors: r+ af1a3ff

…ition, r=alexcrichton Remove data structure specialization for .zip() iterator Go back on half the specialization, the part that changed the Zip struct's fields themselves depending on the types of the iterators. Previous PR: rust-lang#33090 This means that the Zip iterator will always carry two usize fields, which are sometimes unused. If a whole for loop using a .zip() iterator is inlined, these are simply removed and have no effect. The same improvement for Zip of for example slice iterators remain, and they still optimize well. However, like when the specialization of zip was merged, the compiler is still very sensistive to the exact context. For example this code only autovectorizes if the function is used, not if the code in zip_sum_i32 is inserted inline where it was called: ```rust fn zip_sum_i32(xs: &[i32], ys: &[i32]) -> i32 { let mut s = 0; for (&x, &y) in xs.iter().zip(ys) { s += x * y; } s } fn zipdot_i32_default_zip(b: &mut test::Bencher) { let xs = vec![1; 1024]; let ys = vec![1; 1024]; b.iter(|| { zip_sum_i32(&xs, &ys) }) } ``` Include a test that checks that `Zip<T, U>` is covariant w.r.t. T and U. Fixes rust-lang#35727

bors · 2016-09-17T15:23:44Z

⌛ Testing commit af1a3ff with merge fb62f4d...

…excrichton Remove data structure specialization for .zip() iterator Go back on half the specialization, the part that changed the Zip struct's fields themselves depending on the types of the iterators. Previous PR: #33090 This means that the Zip iterator will always carry two usize fields, which are sometimes unused. If a whole for loop using a .zip() iterator is inlined, these are simply removed and have no effect. The same improvement for Zip of for example slice iterators remain, and they still optimize well. However, like when the specialization of zip was merged, the compiler is still very sensistive to the exact context. For example this code only autovectorizes if the function is used, not if the code in zip_sum_i32 is inserted inline where it was called: ```rust fn zip_sum_i32(xs: &[i32], ys: &[i32]) -> i32 { let mut s = 0; for (&x, &y) in xs.iter().zip(ys) { s += x * y; } s } fn zipdot_i32_default_zip(b: &mut test::Bencher) { let xs = vec![1; 1024]; let ys = vec![1; 1024]; b.iter(|| { zip_sum_i32(&xs, &ys) }) } ``` Include a test that checks that `Zip<T, U>` is covariant w.r.t. T and U. Fixes #35727

bors · 2016-09-17T19:02:32Z

rust-highfive assigned alexcrichton Sep 15, 2016

pnkfelix approved these changes Sep 15, 2016

View reviewed changes

bluss force-pushed the zip-slightly-despecialized-edition branch from 07d4213 to af1a3ff Compare September 15, 2016 11:00

nagisa requested changes Sep 15, 2016

View reviewed changes

brson added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 16, 2016

alexcrichton added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Sep 16, 2016

Manishearth mentioned this pull request Sep 17, 2016

Rollup of 5 pull requests #36552

Closed

bors merged commit af1a3ff into rust-lang:master Sep 17, 2016

bluss deleted the zip-slightly-despecialized-edition branch September 19, 2016 12:40

brson mentioned this pull request Sep 19, 2016

Beta backports #36591

Merged

brson removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 20, 2016

timvermeulen mentioned this pull request Mar 25, 2020

Make iter::Fuse truly zero cost #70374

Closed

cuviper mentioned this pull request Mar 29, 2020

Specialize internal state of Fuse depending on underlying iterator #70502

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove data structure specialization for .zip() iterator #36490

Remove data structure specialization for .zip() iterator #36490

bluss commented Sep 15, 2016 •

edited

Loading

rust-highfive commented Sep 15, 2016

pnkfelix Sep 15, 2016

bluss Sep 15, 2016

pnkfelix commented Sep 15, 2016

nagisa Sep 15, 2016 •

edited

Loading

bluss Sep 15, 2016

nagisa Sep 15, 2016

bluss Sep 15, 2016

nagisa commented Sep 15, 2016 •

edited

Loading

bluss commented Sep 15, 2016

bluss commented Sep 15, 2016

brson commented Sep 16, 2016

alexcrichton commented Sep 16, 2016

bors commented Sep 17, 2016

bors commented Sep 17, 2016

		@@ -0,0 +1,17 @@
		// Copyright 2015 The Rust Project Developers. See the COPYRIGHT

Remove data structure specialization for .zip() iterator #36490

Remove data structure specialization for .zip() iterator #36490

Conversation

bluss commented Sep 15, 2016 • edited Loading

rust-highfive commented Sep 15, 2016

pnkfelix Sep 15, 2016

Choose a reason for hiding this comment

bluss Sep 15, 2016

Choose a reason for hiding this comment

pnkfelix commented Sep 15, 2016

nagisa Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

bluss Sep 15, 2016

Choose a reason for hiding this comment

nagisa Sep 15, 2016

Choose a reason for hiding this comment

bluss Sep 15, 2016

Choose a reason for hiding this comment

nagisa commented Sep 15, 2016 • edited Loading

bluss commented Sep 15, 2016

bluss commented Sep 15, 2016

brson commented Sep 16, 2016

alexcrichton commented Sep 16, 2016

bors commented Sep 17, 2016

bors commented Sep 17, 2016

bluss commented Sep 15, 2016 •

edited

Loading

nagisa Sep 15, 2016 •

edited

Loading

nagisa commented Sep 15, 2016 •

edited

Loading