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 zip_eq adapter to IndexedParallelIterator (ZipEq struct version) #392

Merged
merged 5 commits into from Sep 22, 2017

Conversation

Projects
None yet
3 participants
@chrisvittal
Copy link
Contributor

chrisvittal commented Jun 30, 2017

This replaces #389, and is much the same. Re-implemented zip_eq using a ZipEq struct that is a wrapper around Zip to better align zip_eq with the adapters present in rayon, itertools, and the standard library.

Closes #386, as I believe it has been decided to not implement zip_longest.

@cuviper it was unclear from your comments where you wanted additional documentation. Please advise and I will update.

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented Jun 30, 2017

it was unclear from your comments where you wanted additional documentation.

On struct ZipEq, since this is public. Doesn't need to be a lot, just something relating it back to the zip_eq method.

@chrisvittal

This comment has been minimized.

Copy link
Contributor Author

chrisvittal commented Jun 30, 2017

I've been attempting to diagnose CI failures on nightly, and it appears that compiletest_rs is broken on current nightly (rustc 1.20.0-nightly (3bfc18a96 2017-06-29)) on mac and linux. I will investigate more when I get the chance.

The nightly before today works fine.

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented Jun 30, 2017

It seems compiletest_rs 0.2.8 fixed that issue. I restarted your nightly CI runs, and Linux passed, but macOS is still queued...

@chrisvittal

This comment has been minimized.

Copy link
Contributor Author

chrisvittal commented Jun 30, 2017

In the demo, there are dependencies on both cocoa-0.3.3 and cocoa-0.5.2, which both fail to build on current nightly. It seems that servo/cocoa-rs#166 fixed this for the current cocoa-rs (0.9.2), but I'm not sure what we can do here.

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented Jun 30, 2017

Well that's awkward. I wonder what caused the change in Sized behavior -- @nikomatsakis ?

Our dependency tree looks something like this:

  • rayon-demo
    • glium
      • glutin
        • cocoa 0.3
        • winit
          • cocoa 0.5

Perhaps we should just skip rayon-demo for CI on macOS. We'll still have the basic rayon tests, but we don't need to get in the business of troubleshooting graphics stacks here...

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Jul 30, 2017

I think that this is better than zip_eq in itertools, because this is checking up front and then iterating unhindered.

itertools' zip_eq is lazy (only can panic if it reaches an end) and consequently gets in the way of good loop optimizations since it's putting a panic check between each iterator element.

If itertools changes it might be to change zip_eq to apply in just the cases where we can check iterator length up front (a passing thought, it might then exclude exactly those cases, like when filtering, where the check is most crucial.. hmm).

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented Sep 17, 2017

Can you please rebase this to see how CI fares now?

chrisvittal added some commits Jun 29, 2017

Add zip_eq iterator adapter
Implement a rayon version of zip_eq, and the ZipEq type. Do this by
creating a wrapper (the ZipEq struct) around Zip and then reusing all of
Zip's machinery.

@chrisvittal chrisvittal force-pushed the chrisvittal:zip_eq branch from 7a140e7 to 99c68a8 Sep 18, 2017

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented Sep 22, 2017

Thanks!

@cuviper cuviper merged commit 3b778f5 into rayon-rs:master Sep 22, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@chrisvittal chrisvittal deleted the chrisvittal:zip_eq branch Sep 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.