Bump futures to 0.1.16, move to `Notify` API #357

leodasvacas · Jun 5, 2017

Fixes #356. See that issue for previous discussion. Ping @cuviper.

The Notify API is not fond of Arc<Notify> as the old API was with Arc<Unpark>. Instead it prefers to take a generic T: Into<UnsafeNotify>. Because of the 'static restriction in the implementation provided by futures-rs we need to roll our own implementation of UnsafeNotify and jump through extra hoops to hide the lifetimes in ScopeFuture.

The implementation is easier to follow if you consider that it is analogous to the implementation for Arc in futures-rs.

As a bright side, the old make_unpark is gone along with the unpark field, we now simply reuse the this self reference.

There are probably many places where the unpark terminology should be switched to notify, I should also update the great README.md.

lilianmoraru · Jun 8, 2017

On AppVeyor seems like a Rust MinGW build issue(nightly regression?): https://ci.appveyor.com/project/lilianmoraru/rayon/build/1.0.4

In that build, this is being tested: master...lilianmoraru:master

Pinging @nikomatsakis to raise awareness of a probable nightly regression.

cuviper · Jun 8, 2017

Hmm, similar gcc issues were reported in the users forum:
https://users.rust-lang.org/t/rustup-beta-nightly-linker-error-on-windows-10/11236

lilianmoraru · Jun 9, 2017

The issue is fixed on the latest nightly(not sure about beta but it is not relevant to this pull request since it already passed that test): https://ci.appveyor.com/project/lilianmoraru/rayon/build/1.0.5

nikomatsakis · Jun 10, 2017

I'd rather make these changes atop of #344

nikomatsakis · Jun 10, 2017

I'll have to review this Notify API. This feels like a lot of boilerplate, but maybe it's necessary.

leodasvacas · Jun 10, 2017

Sure I can rebase this on #344 once it lands.

cuviper · Sep 16, 2017

We now have separated rayon-futures -- please do rebase on that!

leodasvacas · Sep 17, 2017

@cuviper Thanks for the heads up, rebased!

cuviper · Sep 17, 2017

cuviper reviewed Sep 17, 2017

View changes

cuviper · Sep 17, 2017

Cargo.toml

@@ -23,6 +23,6 @@ default-features = false
 [dev-dependencies]
 compiletest_rs = "0.2.1"
 docopt = "0.7"
-futures = "0.1.7"
+futures = "0.1.14"


I missed this dependency in the shuffle, but I think it can just go away, as rayon-futures now tests itself.

nikomatsakis · Sep 22, 2017

OK so this looks pretty reasonable! I'll be honest, I'm having a bit of a hard time wrapping my head around it, in part because I'm just a bit tired right now. =) @leodasvacas did you want to update the README.md etc?

nikomatsakis · Sep 22, 2017

nikomatsakis approved these changes Sep 22, 2017

View changes

leodasvacas · Sep 22, 2017

@nikomatsakis Nice, I was just looking for an OK before updating the README, will do.

cuviper · Sep 22, 2017

The possessive "its" (without an apostrophe) was correct before.

leodasvacas · Sep 22, 2017

@cuviper Fixed it thanks!

tmccombs · Sep 25, 2017

tmccombs approved these changes Sep 25, 2017

View changes

tmccombs · Sep 25, 2017

rayon-futures/README.md

@@ -227,21 +226,21 @@ This is good, because there *are* still active refs to the
 `ScopeFuture` after we enter the *COMPLETE* state. There are two
 sources of these: unpark values and the future result.

-**Unpark values.** We may have given away `Arc<Unpark>` values --
+**NotifyHandle values.** We may have given away `NotifyHandle` values --
 these are trait objects, but they are actually refs to our


technically NotifyHandle is a struct, not a trait object, although it does contain a raw pointer to a trait object, so the old text sort of still applies.

tmccombs · Sep 25, 2017

rayon-futures/src/lib.rs

+    F: Future + Send + 'scope,
+    S: ScopeHandle<'scope>;
+
+impl<'scope, F, S> Clone for ArcScopeFuture<'scope, F, S>


Couldn't this implementation be derived?

Nope, derive isn't smart enough I think it derives only when F: Clone and S: Clone. See rust-lang/rust#26925.

tmccombs · Sep 25, 2017

rayon-futures/src/lib.rs

+        self.0.notify(id)
+    }
+
+    fn clone_id(&self, id: usize) -> usize {


I don't think you really need to implement clone_id and drop_id, since you don't ever use a meaningful id that needs to be cloned or dropped specially.

tmccombs · Sep 25, 2017

rayon-futures/src/lib.rs

+        }
+    }
+
+    fn clone_id(&self, id: usize) -> usize {


Again I don't think you need to implement clone_id and drop_id here.

leodasvacas · Sep 26, 2017

@tmccombs Thank you very much for the review! I've clarified the README. Regarding the clone_id and drop_id impls for ArcScopeFuture and ScopeFutureWrapped, while they will they might be useless for us and using the default would happen to be equivalent to the Notify impl for ScopeFuture, I believe the correct thing is to make them explicitly call that impl.

nikomatsakis · Oct 17, 2017

bors r+

I've got to dig more into the notify APIs and stuff myself, but this looks really good. We can continue iterating here regardless. Thanks @leodasvacas very much and sorry about the delays.

nikomatsakis · Oct 17, 2017

PS thanks to @tmccombs for the review as well. =) It's good to have more eyes on this.

bors · Oct 17, 2017

Timed out

cuviper · Oct 18, 2017

bors retry

cuviper · Oct 18, 2017

bors r=nikomatsakis

bors · Oct 18, 2017

Build failed

continuous-integration/travis-ci/push

cuviper · Oct 18, 2017

Looks like a corrupted rustc install on OSX stable.

Third time's the charm, or I'll merge it manually:

@bors r=nikomatsakis

notriddle · Oct 18, 2017

Drop the @-sign.

cuviper · Oct 18, 2017

Bah, thanks.

bors r=nikomatsakis

bors · Oct 19, 2017

Timed out

cuviper requested a review from nikomatsakis Sep 17, 2017

leodasvacas changed the title ~~Bump futures to 0.1.14, move to `Notify` API~~ Bump futures to 0.1.16, move to `Notify` API Sep 22, 2017

Fix missing Debug implementations, revert wrong README grammar change

Loading status checks…

cc91eaf

cuviper referenced this pull request Sep 25, 2017
Closed
Transition from deprecated Unpark to Notify #447

clarify rayon-futures README

Loading status checks…

4ca975f

rayon-rs/rayon

Join GitHub today

Bump futures to 0.1.16, move to `Notify` API #357

Conversation

leodasvacas commented Jun 5, 2017

This comment has been minimized.

lilianmoraru commented Jun 8, 2017

This comment has been minimized.

cuviper commented Jun 8, 2017

This comment has been minimized.

lilianmoraru commented Jun 9, 2017

This comment has been minimized.

nikomatsakis commented Jun 10, 2017

This comment has been minimized.

nikomatsakis commented Jun 10, 2017

This comment has been minimized.

leodasvacas commented Jun 10, 2017 • edited

This comment has been minimized.

cuviper commented Sep 16, 2017

This comment has been minimized.

leodasvacas commented Sep 17, 2017

cuviper requested a review from nikomatsakis Sep 17, 2017

cuviper reviewed Sep 17, 2017 View changes

This comment has been minimized.

cuviper Sep 17, 2017

This comment has been minimized.

nikomatsakis commented Sep 22, 2017

nikomatsakis approved these changes Sep 22, 2017 View changes

This comment has been minimized.

leodasvacas commented Sep 22, 2017

leodasvacas changed the title Bump futures to 0.1.14, move to `Notify` API Bump futures to 0.1.16, move to `Notify` API Sep 22, 2017

leodasvacas added some commits Sep 17, 2017

This comment has been minimized.

cuviper commented on rayon-futures/README.md in 60f95dd Sep 22, 2017

This comment has been minimized.

leodasvacas commented Sep 22, 2017

cuviper referenced this pull request Sep 25, 2017

Transition from deprecated Unpark to Notify #447

tmccombs approved these changes Sep 25, 2017 View changes

This comment has been minimized.

tmccombs Sep 25, 2017

This comment has been minimized.

tmccombs Sep 25, 2017

This comment has been minimized.

leodasvacas Sep 26, 2017

This comment has been minimized.

tmccombs Sep 25, 2017

This comment has been minimized.

tmccombs Sep 25, 2017

This comment has been minimized.

leodasvacas commented Sep 26, 2017

This comment has been minimized.

nikomatsakis commented Oct 17, 2017

bors bot added a commit that referenced this pull request Oct 17, 2017

This comment has been minimized.

nikomatsakis commented Oct 17, 2017

This comment has been minimized.

bors bot commented Oct 17, 2017

Timed out

This comment has been minimized.

cuviper commented Oct 18, 2017

This comment has been minimized.

cuviper commented Oct 18, 2017

bors bot added a commit that referenced this pull request Oct 18, 2017

This comment has been minimized.

bors bot commented Oct 18, 2017

Build failed

This comment has been minimized.

cuviper commented Oct 18, 2017

This comment has been minimized.

notriddle commented Oct 18, 2017

This comment has been minimized.

cuviper commented Oct 18, 2017

bors bot added a commit that referenced this pull request Oct 18, 2017

This comment has been minimized.

bors bot commented Oct 19, 2017

Timed out

Hide details View details cuviper merged commit 780489b into rayon-rs:master Oct 19, 2017 2 of 4 checks passed

2 of 4 checks passed

leodasvacas commented Jun 10, 2017 •

edited

cuviper reviewed Sep 17, 2017

View changes

nikomatsakis approved these changes Sep 22, 2017

View changes

cuviper commented on `rayon-futures/README.md` in `60f95dd` Sep 22, 2017

tmccombs approved these changes Sep 25, 2017

View changes

cuviper merged commit `780489b` into rayon-rs:master Oct 19, 2017
2 of 4 checks passed