Implemented batch dispatching #147

AndreaCatania · 2019-07-05T15:44:16Z

This is the second iteration to implement the Batch dispatching following the feedback received in this PR: #144 (Was easier for me redo everything rather change it)

This is the project to test this feature:
shred_test.zip

WaDelma · 2019-07-06T12:54:12Z

Could the project be an example? As in add it to the examples folder.

AndreaCatania · 2019-07-06T15:07:44Z

Done!

torkleyy

Thank you for your work! It's already pretty good. I did find a bunch of nits, most of them I created a suggestion for which you can just apply from GitHub.

We still need some tests to cover the new code. The things that definitely need testing are:

setup is working correctly
batch with conflicting dependencies does not run in parallel with other systems
batch without conflicting dependencies does run in parallel with other systems
fetching works correctly
dispatching works, and the batch and its systems get executed

examples/batch_dispatching.rs

src/dispatch/batch_builder.rs

Co-Authored-By: Thomas Schaller <torkleyy@gmail.com>

AndreaCatania · 2019-07-10T16:22:18Z

@torkleyy I did all the changes that you advised.

ThreadPool

About the ThreadPool assignment problem that I mentioned you before, I found a solution that seems nice and elegant.

I've created a struct, called ThreadPoolWrapper, which is shared across all batches.
This wrapper contains the actual ThreadPool that now can be modified at any point.

The result of this addition is that when in amethyst the Application struct create the thread pool and assign it to the main DispatcherBuilder it change the internal part of the wrapper and everyone is automatically updated.

To me this seems clear enough but do you think that exist a better way to handle it?

Tests

I written three tests that demonstrate what you suggested, please let me know if I have to add more.

Commit squash

Do you want that I squash all this changes to a single commit?

WaDelma

Looks good! There is some stuff I am not sure about as I haven't looked at the use case for this.

src/dispatch/async_dispatcher.rs

src/dispatch/batch.rs

examples/batch_dispatching.rs

src/dispatch/batch.rs

src/dispatch/dispatcher.rs

src/dispatch/stage.rs

src/dispatch/builder.rs

examples/batch_dispatching.rs

AndreaCatania · 2019-07-13T15:41:17Z

@WaDelma I just did a commit to resolve the things that you pointed out

src/dispatch/builder.rs

Co-Authored-By: WaDelma <WaDelma@users.noreply.github.com>

AndreaCatania · 2019-07-19T09:56:30Z

Any news for this?

Cargo.toml

bors · 2019-08-02T05:22:25Z

✌️ jojolepro can now approve this pull request

torkleyy

Thanks, looks good to me!

bors delegate=jojolepro

torkleyy · 2019-08-02T06:34:14Z

bors r+

147: Implemented batch dispatching r=torkleyy a=AndreaCatania This is the second iteration to implement the Batch dispatching following the feedback received in this PR: #144 (Was easier for me redo everything rather change it) This is the project to test this feature: [shred_test.zip](https://github.com/slide-rs/shred/files/3363344/shred_test.zip) Co-authored-by: Andrea Catania <info@andreacatania.com>

bors · 2019-08-02T06:39:58Z

Build succeeded

continuous-integration/travis-ci/push

AndreaCatania · 2019-08-02T06:41:25Z

Guys, thank you a lot!

AndreaCatania · 2019-08-02T07:47:10Z

I would like to update shred in Amethyst and so use this new feature.
@torkleyy can I help you to add a new version in crates.io?

AnneKitsune · 2019-08-02T10:49:01Z

I'd just like to precise that I reviewed only half of the PR yesterday (lack of time), which is why I didn't add an "accept" review thingy. However, it as been reviewed by 2 people, so I would tend to think that I would not have found more than 1-2 nitpicks in the second half.

AndreaCatania · 2019-08-06T05:33:16Z

@torkleyy, @Jojolepro any idea about how to create a new version in crates.io?
otherwise I can't use it

AnneKitsune · 2019-08-06T12:09:29Z

It depends when torkleyy actually wants to release. I'll ask him.

In the meantime, you can target the git repo using cargo using:

shred = { git = "https://github.com/slide-rs/shred" }
specs = { git = "https://github.com/your_specs_fork" }

For that, you'll need to fork specs and update that Cargo.toml file too so that it targets this repo.

AndreaCatania · 2019-08-09T14:25:13Z

@Jojolepro Thanks for the advice! I wanted to avoid it but that's ok, I can continue to work on phythyst now.

AndreaCatania mentioned this pull request Jul 5, 2019

Batch processing #144

Closed

Implemented batch dispatching

45da66d

AndreaCatania force-pushed the batch_2 branch from e609bec to 45da66d Compare July 6, 2019 15:07

torkleyy reviewed Jul 8, 2019

View reviewed changes

torkleyy added the feature label Jul 8, 2019

torkleyy requested a review from WaDelma July 8, 2019 17:50

AndreaCatania and others added 16 commits July 8, 2019 19:52

Update src/dispatch/batch_builder.rs

d257607

Co-Authored-By: Thomas Schaller <torkleyy@gmail.com>

Update src/dispatch/batch_builder.rs

fc79eac

Co-Authored-By: Thomas Schaller <torkleyy@gmail.com>

Update src/dispatch/batch_builder.rs

918c64b

Co-Authored-By: Thomas Schaller <torkleyy@gmail.com>

Update src/dispatch/batch_builder.rs

e80db47

Co-Authored-By: Thomas Schaller <torkleyy@gmail.com>

Update src/dispatch/batch_builder.rs

e6db203

Co-Authored-By: Thomas Schaller <torkleyy@gmail.com>

Update src/dispatch/batch_builder.rs

a5f5c8a

Co-Authored-By: Thomas Schaller <torkleyy@gmail.com>

Update src/dispatch/batch_builder.rs

c2f7d0a

Co-Authored-By: Thomas Schaller <torkleyy@gmail.com>

Update src/dispatch/batch_builder.rs

38e2117

Co-Authored-By: Thomas Schaller <torkleyy@gmail.com>

Update src/dispatch/batch_builder.rs

6006ce9

Co-Authored-By: Thomas Schaller <torkleyy@gmail.com>

Removed BatchBuilder in favour of a single function

90ff60c

Implemented shareable threa pool

b4c54cf

Mentioned cross dispatching not allowed

cc12f5a

Removed global import

d6b9cbf

Added backticks

d0cd979

Updated example

290f994

Added batch tests

46dbfba

WaDelma reviewed Jul 11, 2019

View reviewed changes

WaDelma reviewed Jul 12, 2019

View reviewed changes

examples/batch_dispatching.rs Show resolved Hide resolved

WaDelma adjustment

6c558a9

WaDelma reviewed Jul 13, 2019

View reviewed changes

src/dispatch/builder.rs Outdated Show resolved Hide resolved

AndreaCatania and others added 12 commits July 14, 2019 14:30

Update src/dispatch/batch.rs

1a930a0

Co-Authored-By: WaDelma <WaDelma@users.noreply.github.com>

Update src/dispatch/batch.rs

b75cfd9

Co-Authored-By: WaDelma <WaDelma@users.noreply.github.com>

Update src/dispatch/batch.rs

9a37f38

Co-Authored-By: WaDelma <WaDelma@users.noreply.github.com>

Update src/dispatch/batch.rs

7e8927e

Co-Authored-By: WaDelma <WaDelma@users.noreply.github.com>

Update src/dispatch/batch.rs

0192eb2

Co-Authored-By: WaDelma <WaDelma@users.noreply.github.com>

Update src/dispatch/batch.rs

f6ae35b

Co-Authored-By: WaDelma <WaDelma@users.noreply.github.com>

Update src/dispatch/batch.rs

e84725e

Co-Authored-By: WaDelma <WaDelma@users.noreply.github.com>

Update src/dispatch/batch.rs

13a27d0

Co-Authored-By: WaDelma <WaDelma@users.noreply.github.com>

Update src/dispatch/batch.rs

bdcd242

Co-Authored-By: WaDelma <WaDelma@users.noreply.github.com>

Update src/dispatch/batch.rs

015616d

Co-Authored-By: WaDelma <WaDelma@users.noreply.github.com>

Update src/dispatch/batch.rs

9516ecf

Co-Authored-By: WaDelma <WaDelma@users.noreply.github.com>

Improved BatchSystemData doc

c945cb3

AnneKitsune reviewed Aug 1, 2019

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

torkleyy approved these changes Aug 2, 2019

View reviewed changes

Added missing new line on the Cargo.toml file

cc981d0

bors bot merged commit cc981d0 into amethyst:master Aug 2, 2019

AndreaCatania deleted the batch_2 branch August 2, 2019 06:40

torkleyy mentioned this pull request Aug 26, 2019

Ability to suspend Systems #151

Open

azriel91 mentioned this pull request Feb 27, 2020

Better documentation/example about BatchController amethyst/specs#689

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented batch dispatching #147

Implemented batch dispatching #147

AndreaCatania commented Jul 5, 2019

WaDelma commented Jul 6, 2019

AndreaCatania commented Jul 6, 2019

torkleyy left a comment

AndreaCatania commented Jul 10, 2019 •

edited

WaDelma left a comment

AndreaCatania commented Jul 13, 2019

AndreaCatania commented Jul 19, 2019

bors bot commented Aug 2, 2019

torkleyy left a comment

torkleyy commented Aug 2, 2019

bors bot commented Aug 2, 2019

AndreaCatania commented Aug 2, 2019 •

edited

AndreaCatania commented Aug 2, 2019

AnneKitsune commented Aug 2, 2019

AndreaCatania commented Aug 6, 2019

AnneKitsune commented Aug 6, 2019

AndreaCatania commented Aug 9, 2019

Implemented batch dispatching #147

Implemented batch dispatching #147

Conversation

AndreaCatania commented Jul 5, 2019

WaDelma commented Jul 6, 2019

AndreaCatania commented Jul 6, 2019

torkleyy left a comment

Choose a reason for hiding this comment

AndreaCatania commented Jul 10, 2019 • edited

ThreadPool

Tests

Commit squash

WaDelma left a comment

Choose a reason for hiding this comment

AndreaCatania commented Jul 13, 2019

AndreaCatania commented Jul 19, 2019

bors bot commented Aug 2, 2019

torkleyy left a comment

Choose a reason for hiding this comment

torkleyy commented Aug 2, 2019

bors bot commented Aug 2, 2019

Build succeeded

AndreaCatania commented Aug 2, 2019 • edited

AndreaCatania commented Aug 2, 2019

AnneKitsune commented Aug 2, 2019

AndreaCatania commented Aug 6, 2019

AnneKitsune commented Aug 6, 2019

AndreaCatania commented Aug 9, 2019

AndreaCatania commented Jul 10, 2019 •

edited

AndreaCatania commented Aug 2, 2019 •

edited