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

Add convenience method for polling Task<> #4627

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

TheRawMeatball
Copy link
Member

Objective

Checking if a task is completed and retrieving the value is too convoluted

Solution

Add a simple convenience method

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Apr 28, 2022
@TheRawMeatball TheRawMeatball added C-Enhancement A new feature A-Tasks Tools for parallel and async work and removed S-Needs-Triage This issue needs to be labelled labels Apr 28, 2022
@hymm
Copy link
Contributor

hymm commented Apr 28, 2022

Is this an alternative to #4102?

If it is what are the advantages/disadvantages vs that PR?

@TheRawMeatball
Copy link
Member Author

Lower overhead, as this doesn't require adding an extra channel, instead using what's already built into async_executor::Task

@DJMcNab
Copy link
Member

DJMcNab commented Apr 28, 2022

But if checking it is more expensive, then I would think that cost would dwarf the extra overhead of the channel.

That is, I can't just let the claim of 'lower overhead' meaning better go by without confirmation - Task::poll isn't a trivial function, and has more inherent overhead than a one-shot channel - because it must behave properly when an actual proper future is awaiting it. Maybe counting moves of the value changes that equation, but I don't find it as obviously clear cut as you.

@alice-i-cecile alice-i-cecile added C-Usability A simple quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR and removed C-Enhancement A new feature labels May 16, 2022
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.

Added the Controversial tag. I'd like to see perf testing here. The API is really simple and appealing, but I don't want that to turn into a footgun.

@hymm
Copy link
Contributor

hymm commented May 16, 2022

I added perf numbers here #4102 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Tasks Tools for parallel and async work C-Usability A simple quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants