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

Use "parallel" feature flag #353

Closed
wants to merge 2 commits into from
Closed

Conversation

BartAdv
Copy link

@BartAdv BartAdv commented Feb 21, 2018

This allows it to be built for targets such as wasm32


This change is Reviewable

@BartAdv
Copy link
Author

BartAdv commented Feb 21, 2018

Hm, it still doesn't build when referenced in my project via specs = { git = "https://github.com/bartadv/specs", rev = "b43e8623165685b78b3b8a87a7fc52faa41c8612", default-features = false }. Ehh those features, I still don't feel like grokking it :)

Anyway, I feel like there's some cleanup to be done in features in Cargo.toml here. I've added default feature, but I'm not sure what's the reason behind common feature - was it meant to be default one?

@BartAdv BartAdv mentioned this pull request Feb 21, 2018
Copy link
Member

@torkleyy torkleyy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise 👍 Thank you!

Cargo.toml Outdated
mopa = "0.2"
shred = "0.6.0"
shrev = "0.8.0"
shred-derive = "0.4"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you accidentally deleted this line.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, restored

@torkleyy
Copy link
Member

torkleyy commented Feb 23, 2018

Also, maybe we can add a Travis job to ensure Specs builds for wasm32 in the future.

to control inclusion of functionality requiring multithreading.
@BartAdv
Copy link
Author

BartAdv commented Feb 23, 2018

Note - I'd still like to check what is the cause of the build problem in my project, whether it requires some attention in specs Cargo.toml or not. Hadn't got time for this lately though ...

@BartAdv
Copy link
Author

BartAdv commented Feb 23, 2018

OK, it was just missing default-features = false on shred dependency...

@@ -163,6 +168,7 @@ pub trait Join {
/// The purpose of the `ParJoin` trait is to provide a way
/// to access multiple storages in parallel at the same time with
/// the merged bit set.
#[cfg(feature = "parallel")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meh, this isn't really ideal. One should be able to compile just the same code for wasm, but this disallows ParJoin. Instead we'd need a fallback for it. But I'm not sure if we can use rayon at all on wasm :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that rayon should build with wasm (rayon-rs/rayon#503), but as wasm doesn't support threads it just panics when using it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, what's the point of being able to compile something only for it to blow up later? I thought this behaviour of wasm32-unknown-unknown target is something that's just done so that it can be shipped earlier. Anyhow, I can change it if you wish

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless the fallback would be to define ParJoin as Join?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't implying we should use it, but just providing info.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, what's the point of being able to compile something only for it to blow up later?

No, the point is it shouldn't fail but fall back to sequential joining. The problem with "defining ParJoin as Join" is that ParJoin has a different (rayon) API.

@torkleyy
Copy link
Member

I'm planning on getting out a new Specs version soon, is anybody still interested in fixing this? It's not really a priority for me right now, although I'd like to target wasm in the future.

@torkleyy
Copy link
Member

How to fix this issue

  • update rayon to fall back to sequential iteration on wasm
  • update this PR

@torkleyy torkleyy mentioned this pull request Apr 22, 2018
9 tasks
@torkleyy
Copy link
Member

torkleyy commented Sep 7, 2018

Closing due to inactivity.

@torkleyy torkleyy closed this Sep 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants