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

Callback Queue: Future support #1125

Merged
merged 6 commits into from Nov 13, 2018

Conversation

Projects
None yet
5 participants
@jojolepro
Copy link
Member

jojolepro commented Nov 10, 2018

This adds support to use futures (and any other asynchronous context) as simple callbacks.

If you want a more in-depth explanation, you can read the rfc here amethyst/rfcs#4
Be aware that you need to know how async computing (futures & others) work to understand it.

jojolepro added some commits Nov 10, 2018

@jojolepro jojolepro requested a review from LucioFranco Nov 10, 2018

@jojolepro

This comment has been minimized.

Copy link
Member

jojolepro commented Nov 10, 2018

The rfc is just here to explain what I'm doing for those who can't see what problem this pr fixes. Feel free to ask questions there if you think this implementation could be better.

@torkleyy

This comment has been minimized.

Copy link
Member

torkleyy commented Nov 10, 2018

Err..that's not an RFC then.

@jojolepro

This comment has been minimized.

Copy link
Member

jojolepro commented Nov 10, 2018

"Fixed" 👍

@torkleyy
Copy link
Member

torkleyy left a comment

I'm not yet sure if it's worth to pull crossbeam for this, but otherwise I'm okay with the code changes. I still need to review docs.

@jojolepro

This comment has been minimized.

Copy link
Member

jojolepro commented Nov 11, 2018

Crossbeam avoids having a Mutex + EventChannel.

I could use mpsc, but apparently crossbeam's channel is more performant.

@Rhuagh

Rhuagh approved these changes Nov 12, 2018

@LucioFranco

This comment has been minimized.

Copy link
Member

LucioFranco commented Nov 12, 2018

@jojolepro two things, could you possibly create a small example and I would like to see what actual benefits we get from using crossbeam-channel vs just std mpsc. I'd like to not bring in another dep.

@jojolepro

This comment has been minimized.

Copy link
Member

jojolepro commented Nov 12, 2018

  1. Small example.
    https://github.com/amethyst/amethyst/pull/1125/files#diff-870ccdada9e859df05ecd0d7496f31a7R21

There isn't a full fledged example because there is no use case to do this in amethyst without adding dependencies.
The best example I could do would be to spawn a thread that sleeps for a second, and then triggers a Trans::Quit. Do you want me to do that?

  1. Crossbeam
    I didn't really check the differences between crossbeam's channel and mpsc. Someone suggested I use it. They got an explanation on why its better than mpsc here https://docs.rs/crossbeam-channel/0.3.1/crossbeam_channel/
@LucioFranco
Copy link
Member

LucioFranco left a comment

After @jojolepro and @azriel91 discussion on discord, I think this is fine.

@azriel91

This comment has been minimized.

Copy link
Member

azriel91 commented Nov 12, 2018

Summary of discord discussion:

  • crossbeam channels are Send + Sync, we'd need to use std::mpsc::SyncSender if we swap back to std
  • crossbeam (in the past) was profiled to be faster than std channels, not sure of current state
  • crossbeam is already a dependency of one of our dependencies, so we're not adding much compilation time

Leaving this open so @torkleyy can finish review

@torkleyy
Copy link
Member

torkleyy left a comment

Thanks for the summary!
Looks good 👍

bors r+

bors bot added a commit that referenced this pull request Nov 13, 2018

Merge #1125
1125: Callback Queue: Future support r=torkleyy a=jojolepro

This adds support to use futures (and any other asynchronous context) as simple callbacks.

If you want a more in-depth explanation, you can read the rfc here amethyst/rfcs#4
Be aware that you need to know how async computing (futures & others) work to understand it.

Co-authored-by: Joël Lupien (Jojolepro) <jojolepromain@gmail.com>
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Nov 13, 2018

@bors bors bot merged commit d0f9639 into amethyst:master Nov 13, 2018

4 checks passed

bors Build succeeded
Details
concourse-ci/linux-book-tests Concourse CI build success
Details
concourse-ci/linux-tests Concourse CI build success
Details
concourse-ci/rustfmt Concourse CI build success
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment