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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - tick local executor #6121

Closed
wants to merge 9 commits into from
Closed

[Merged by Bors] - tick local executor #6121

wants to merge 9 commits into from

Conversation

hymm
Copy link
Contributor

@hymm hymm commented Sep 28, 2022 •
edited
Loading

Objective

Solution

  • Add system for ticking local executors on main thread into bevy_core where the tasks pools are initialized.
  • Add ticking local executors into thread executors

Changelog

  • tick all thread local executors in task pool.

Notes

  • Not 100% sure about this PR. Ticking the local executor for the main thread in scope feels a little kludgy as it requires users of bevy_tasks to be calling scope periodically for those tasks to make progress. took this out in favor of a system that ticks the local executors.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Tasks Tools for parallel and async work labels Sep 28, 2022
@alice-i-cecile
Copy link
Member

Can we add an integration test with this PR? This code is clearly tricky to reason about and we were caught totally off-guard by the regression.

@maniwani maniwani requested a review from james7132 September 28, 2022 21:50
@maniwani
Copy link
Contributor

maniwani commented Sep 28, 2022 •
edited
Loading

I'm not that acquainted with async to know what a better state would look like tbh. Is it fair to say the issue here is that all LocalExecutors should be ticked whether or not we're in a scope, but this whole time it's only been happening inside a scope?

AFAIK #4466 was our only option to defer system task spawning, given the constraint that the system executor itself has to be a Send task to not block/get blocked by systems that must run on the local thread.

This PR doesn't help if you use AsyncTaskPool::get().spawn_local() or IoTaskPool::get().spawn_local(), since the scope for those is never called.

I think #4740 would fix this particular point. I don't know if it would also fix the kludginess. I'm not familiar with the details.

@hymm
Copy link
Contributor Author

hymm commented Sep 28, 2022

Is it fair to say the issue here is that all LocalExecutors should be ticked whether or not we're in a scope, but this whole time it's only been happening inside a scope?

yeah. The issue is that the LocalExecutor needs to be ticked on the main thread and so has to cooperate with everything else the main thread has to do. It might make more sense to tick all the global task pools inside the app runner instead of the scope and add some docs to the TaskPools that "it might be up to the user to tick the local executor on the main thread" for situations where the user isn't using app or using a custom runner.

@alice-i-cecile alice-i-cecile added this to the Bevy 0.9 milestone Sep 29, 2022
@hymm hymm force-pushed the tick-thread-local-executors branch from 2be5a7c to f95c417 Compare October 4, 2022 18:21
alice-i-cecile
alice-i-cecile approved these changes Oct 4, 2022
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

LGTM on the code quality and docs side. I don't have the expertise on the task side to give a full review, so wait on a second approval from someone who does.

@hymm
Copy link
Contributor Author

hymm commented Oct 4, 2022

I still want to add a test for ticking the local executors, so please wait on that before merging.

@hymm
Copy link
Contributor Author

hymm commented Oct 4, 2022

I still want to add a test for ticking the local executors, so please wait on that before merging.

Added a test.

james7132
james7132 approved these changes Oct 19, 2022
View reviewed changes
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

I really don't like that we need to add a system to tick the local executors, it seems like a last minute monkeypatch. I think this would be easier if we cracked open the internals of async_executor (a la #4740), but this seems to work for now.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Oct 19, 2022
@alice-i-cecile
Copy link
Member

@hymm can you rebase this? :)

@hymm
Copy link
Contributor Author

hymm commented Oct 19, 2022 •
edited
Loading

I really don't like that we need to add a system to tick the local executors, it seems like a last minute monkeypatch. I think this would be easier if we cracked open the internals of async_executor (a la #4740), but this seems to work for now.

I consider the current solution to be the most flexible. There's some ambition to move the bevy runner off of the main thread. This is for multiple reasons including the fact that you can't block (i.e. mutex) on the main thread on wasm. In this case we'll probably need a way of ticking the local executors on the main thread that is not tied to running the scope. This could potentially be a system or some type of main thread runner. In either case the function added in the PR can be called to tick the local executors.

alice-i-cecile
alice-i-cecile approved these changes Oct 24, 2022
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Let's unbreak this for 0.9.

Code LGTM, I like the system based approach much better.

bors r+

bors bot pushed a commit that referenced this pull request Oct 24, 2022
@hymm
tick local executor (#6121)
0f3f628 
# Objective

- #4466 broke local tasks running.
- Fixes #6120

## Solution

- Add system for ticking local executors on main thread into bevy_core where the tasks pools are initialized.
- Add ticking local executors into thread executors

## Changelog

- tick all thread local executors in task pool.

## Notes

- ~~Not 100% sure about this PR. Ticking the local executor for the main thread in scope feels a little kludgy as it requires users of bevy_tasks to be calling scope periodically for those tasks to make progress.~~ took this out in favor of a system that ticks the local executors.
@bors
Copy link
Contributor

bors bot commented Oct 24, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title tick local executor [Merged by Bors] - tick local executor Oct 24, 2022
@bors bors bot closed this Oct 24, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
@hymm @james7132
tick local executor (bevyengine#6121)
c5e9302 
# Objective

- bevyengine#4466 broke local tasks running.
- Fixes bevyengine#6120

## Solution

- Add system for ticking local executors on main thread into bevy_core where the tasks pools are initialized.
- Add ticking local executors into thread executors

## Changelog

- tick all thread local executors in task pool.

## Notes

- ~~Not 100% sure about this PR. Ticking the local executor for the main thread in scope feels a little kludgy as it requires users of bevy_tasks to be calling scope periodically for those tasks to make progress.~~ took this out in favor of a system that ticks the local executors.
Pietrek14 pushed a commit to Pietrek14/bevy that referenced this pull request Dec 17, 2022
@hymm @Pietrek14
tick local executor (bevyengine#6121)
48ca0e9 
# Objective

- bevyengine#4466 broke local tasks running.
- Fixes bevyengine#6120

## Solution

- Add system for ticking local executors on main thread into bevy_core where the tasks pools are initialized.
- Add ticking local executors into thread executors

## Changelog

- tick all thread local executors in task pool.

## Notes

- ~~Not 100% sure about this PR. Ticking the local executor for the main thread in scope feels a little kludgy as it requires users of bevy_tasks to be calling scope periodically for those tasks to make progress.~~ took this out in favor of a system that ticks the local executors.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@hymm @ItsDoot
tick local executor (bevyengine#6121)
0e8310c 
# Objective

- bevyengine#4466 broke local tasks running.
- Fixes bevyengine#6120

## Solution

- Add system for ticking local executors on main thread into bevy_core where the tasks pools are initialized.
- Add ticking local executors into thread executors

## Changelog

- tick all thread local executors in task pool.

## Notes

- ~~Not 100% sure about this PR. Ticking the local executor for the main thread in scope feels a little kludgy as it requires users of bevy_tasks to be calling scope periodically for those tasks to make progress.~~ took this out in favor of a system that ticks the local executors.
@hymm hymm deleted the tick-thread-local-executors branch October 2, 2023 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@james7132 james7132 james7132 approved these changes

@maniwani maniwani Awaiting requested review from maniwani

@TheRawMeatball TheRawMeatball Awaiting requested review from TheRawMeatball

Assignees
No one assigned
Labels
A-Tasks Tools for parallel and async work C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Scheduling
Archived in project
Milestone
Bevy 0.9
Development

Successfully merging this pull request may close these issues.

Detached non-Send tasks don't run
4 participants
@hymm @alice-i-cecile @maniwani @james7132