Remove send+sync requirement if not parallel #674

chinedufn · 2020-01-24T16:18:24Z

Closes #673

chinedufn · 2020-01-24T16:19:32Z

Cargo.toml

@@ -27,7 +27,7 @@ derivative = "1"
 hashbrown = "0.6.0"
 hibitset = { version = "0.6.1", default-features = false }
 log = "0.4"
-shred = { version = "0.9.3", default-features = false }
+shred = { version = "0.10.1", default-features = false }


shred 0.10.1 needs to be deployed before this PR can be merged

amethyst/shred#186

chinedufn · 2020-01-24T16:20:30Z

Cargo.toml

@@ -39,7 +39,6 @@ uuid = { version = "0.7.4", optional = true, features = ["v4", "serde"] }
 [features]
 default = ["parallel"]
 parallel = ["rayon", "shred/parallel", "hibitset/parallel"]
-nightly = ["shred/nightly"]


A breaking change so I bumped the minor version of specs

We could also leave this feature in and just make it nightly = [] if we don't want to bump to 0.16 just yet - then I can just make this PR bump to 0.15.2

Whatever works for specs!

azriel91

Heya, the changes in both PRs look good to these eyes; could you also add a test that the non-"parallel" version builds with a !Send + !Sync type? Maybe just checking that adding a struct NonSend(pub Rc<u32>) to the World would suffice.

azriel91 · 2020-02-03T21:51:44Z

Heya, took a while to get back, but the changes in both PRs look good to me.

In terms of repository ownership, I don't yet feel like I'm "the one", else I'd be happy to take them.
@WaDelma since @torkleyy's focusing on other things, would you mind me making decisions at this level? am also happy to tag you whenever this kind of decision needs to be made (usually directional, rather than technical).

WaDelma · 2020-02-04T07:32:40Z

@chinedufn tbh I have never felt like being "the one" either and have been bit afraid of stepping on other peoples toes accidentally, so I am happy to let you be it. I can participate on things though!

azriel91 · 2020-02-11T00:23:46Z

@chinedufn I've tried getting specs to be tested with the shred change in #677, but Rust isn't happy yet:

$ cargo test --no-default-features --workspace                                                                                                                                                                                                
# ..
error: aborting due to 55 previous errors
# ..

Could you check what's blocking that? I haven't merged shred's PR because I'd like to see it working end-to-end first (instead of merge/release and needing to yank versions).

chinedufn · 2020-02-11T07:47:07Z

@azriel91 sorry about that - d74d02b makes the following pass

git clone https://github.com/chinedufn/specs
cd specs
git checkout origin/non-send-sync-resources
git checkout d74d02b724529994c729ea8a5c4a3f897e2d1655
cargo test --workspace && \
  cargo test --no-default-features --workspace

chinedufn · 2020-02-11T07:49:18Z

examples/cluster_bomb.rs

@@ -1,9 +1,11 @@
 extern crate rand;


Opted for the quick approach to get things passing with the parallel feature disabled by just panicking.

I thought that the work of making the examples work in parallel and non-parallel could be left for the future since these wouldn't have worked in not(parallel) before anyway

chinedufn · 2020-02-11T07:49:33Z

examples/full.rs

@@ -1,4 +1,6 @@
+#[cfg(feature = "parallel")]


Same idea here - quick and dirty approach of panicking when not parallel

chinedufn · 2020-02-11T07:50:10Z

tests/no_parallel.rs

@@ -0,0 +1,14 @@
+#![cfg(not(feature = "parallel"))]


@azriel91 addresses #674 (review) - thanks for the feedback!

azriel91 · 2020-02-11T20:01:21Z

ah oops, I did similar changes in #677 (but didn't force push to your branch -- in case you still wanted it).

oh. just discovered why -- [dev-dependencies] didn't have default-features = false for shred. Shall push that one over this branch soon (shall release shred).

This reverts commit 1ed2912.

azriel91 · 2020-02-12T03:47:49Z

Goal: Support Components that are not Send + Sync.

There is a test in tests/no_parallel.rs to prove the goal is reached when "parallel" is disabled. Existing tests prove when "parallel" is enabled.

Ideal solution: Trait aliases, in beta but not stable, and very little signs of movement.

#![cfg(feature = "parallel")]
trait ComponentBound = Component + Send + Sync;
#![cfg(not(feature = "parallel"))]
trait ComponentBound = Component;

Duplicate code: Copy code per feature = "parallel" and not(feature = "parallel"). Simple but ugly.
Poor man's trait aliases: Implemented in e1477cf, but not ideal, because users have to specify the type of world in their closures: 1ed2912, which is very bad^TM.
Macro to generate per feature fn (current state): Kind of like the duplication option without the duplication, but macro is ugly to maintain.

@chinedufn @WaDelma I think it's good to have both of you review this (again). The summary above should capture the options I've tried.

chinedufn

Looks great - thanks for making this much more robust than what I originally had!

src/world/lazy.rs

chinedufn · 2020-02-12T07:59:43Z

tests/no_parallel.rs

+}
+
+#[test]
+fn non_send_component_is_accepted() {


azriel91 · 2020-02-12T08:22:37Z

Just realized that's no test for a storage that isn't Send, if you like, could you add one? That'll help the PR get through a little faster.

chinedufn · 2020-02-12T08:28:28Z

If the type = VecStorage<Self> and Self isn't send then isn't the storage non send?

Or am I missing something here?

azriel91 · 2020-02-12T08:45:56Z

ah that's correct ✌️ -- it backs onto Vec which only impls Send when T is Send

WaDelma

LGTM

azriel91 · 2020-02-12T20:22:24Z

bors r+

674: Remove send+sync requirement if not parallel r=azriel91 a=chinedufn Closes #673 Co-authored-by: Chinedu Francis Nwafili <[email protected]> Co-authored-by: Azriel Hoh <[email protected]>

bors · 2020-02-12T20:39:52Z

Build succeeded

continuous-integration/travis-ci/push

chinedufn force-pushed the non-send-sync-resources branch from d53219b to 99e4d62 Compare January 24, 2020 16:18

chinedufn mentioned this pull request Jan 24, 2020

Components require Send + Sync even when parallel feature is disabled #673

Closed

chinedufn commented Jan 24, 2020

View reviewed changes

chinedufn force-pushed the non-send-sync-resources branch 2 times, most recently from b1c766c to 9b12b28 Compare January 24, 2020 16:27

azriel91 reviewed Feb 3, 2020

View reviewed changes

azriel91 mentioned this pull request Feb 11, 2020

Non send sync resources #677

Closed

4 tasks

chinedufn commented Feb 11, 2020

View reviewed changes

chinedufn and others added 8 commits February 12, 2020 11:41

Remove send+sync requirement if not parallel

3f7f930

Added test to use shred with "no-parallel" feature.

e079840

Experiment for non-Send/Sync component support.

c9422e2

Allow async example to be built without "parallel" feature.

941b04d

Poor man's trait-alias substitute.

e1477cf

Allow tests to compile with aggregate traits.

1ed2912

Revert "Allow tests to compile with aggregate traits."

60da5c5

This reverts commit 1ed2912.

Use macro to generate feature gate.

b4e83ce

azriel91 force-pushed the non-send-sync-resources branch from e29ef4a to b4e83ce Compare February 12, 2020 03:43

chinedufn commented Feb 12, 2020

View reviewed changes

WaDelma approved these changes Feb 12, 2020

View reviewed changes

Added comment explaining parallel_feature macro.

da350ee

bors bot added a commit that referenced this pull request Feb 12, 2020

Merge #674

385bbda

674: Remove send+sync requirement if not parallel r=azriel91 a=chinedufn Closes #673 Co-authored-by: Chinedu Francis Nwafili <[email protected]> Co-authored-by: Azriel Hoh <[email protected]>

bors bot merged commit da350ee into amethyst:master Feb 12, 2020

chinedufn deleted the non-send-sync-resources branch February 13, 2020 00:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove send+sync requirement if not parallel #674

Remove send+sync requirement if not parallel #674

chinedufn commented Jan 24, 2020

chinedufn Jan 24, 2020 •

edited

Loading

chinedufn Jan 24, 2020

azriel91 left a comment

azriel91 commented Feb 3, 2020

WaDelma commented Feb 4, 2020 •

edited

Loading

azriel91 commented Feb 11, 2020

chinedufn commented Feb 11, 2020 •

edited

Loading

chinedufn Feb 11, 2020

chinedufn Feb 11, 2020

chinedufn Feb 11, 2020

azriel91 commented Feb 11, 2020

azriel91 commented Feb 12, 2020 •

edited

Loading

chinedufn left a comment

chinedufn Feb 12, 2020

azriel91 commented Feb 12, 2020

chinedufn commented Feb 12, 2020

azriel91 commented Feb 12, 2020

WaDelma left a comment

azriel91 commented Feb 12, 2020

bors bot commented Feb 12, 2020

Remove send+sync requirement if not parallel #674

Remove send+sync requirement if not parallel #674

Conversation

chinedufn commented Jan 24, 2020

chinedufn Jan 24, 2020 • edited Loading

Choose a reason for hiding this comment

chinedufn Jan 24, 2020

Choose a reason for hiding this comment

azriel91 left a comment

Choose a reason for hiding this comment

azriel91 commented Feb 3, 2020

WaDelma commented Feb 4, 2020 • edited Loading

azriel91 commented Feb 11, 2020

chinedufn commented Feb 11, 2020 • edited Loading

chinedufn Feb 11, 2020

Choose a reason for hiding this comment

chinedufn Feb 11, 2020

Choose a reason for hiding this comment

chinedufn Feb 11, 2020

Choose a reason for hiding this comment

azriel91 commented Feb 11, 2020

azriel91 commented Feb 12, 2020 • edited Loading

chinedufn left a comment

Choose a reason for hiding this comment

chinedufn Feb 12, 2020

Choose a reason for hiding this comment

azriel91 commented Feb 12, 2020

chinedufn commented Feb 12, 2020

azriel91 commented Feb 12, 2020

WaDelma left a comment

Choose a reason for hiding this comment

azriel91 commented Feb 12, 2020

bors bot commented Feb 12, 2020

Build succeeded

chinedufn Jan 24, 2020 •

edited

Loading

WaDelma commented Feb 4, 2020 •

edited

Loading

chinedufn commented Feb 11, 2020 •

edited

Loading

azriel91 commented Feb 12, 2020 •

edited

Loading