-
Notifications
You must be signed in to change notification settings - Fork 264
Allow IAsync...WithProgress to report intermediate results #870
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
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Clients of IAsync...WithProgress are permitted to call GetResults()
before the operation has completed, allowing them to observe partial
results. This behavior is necessary because the Progress type must
be a value type, but the operation may want to report reference types
in their progress reports. The way to do that is to report the reference
type as the GetResults().
The coroutine itself can use the `set_result()` method on the progress
token to report a result prior to completion, and can update that result
as often as desired prior to the completion of the coroutine. The value
passed to `co_return` acts as the final result of the coroutine. If
`set_result()` is never called, then the intermediate result is an empty
result (null for reference types, default-initialized for value types).
In practice, the result of the coroutine is usually a reference type,
and the common pattern is to create the result object, pass it to
`set_result()` at the start of the coroutine, update the result object
as the coroutine progresses, and (redundantly) pass the same result object
to `co_return` when the coroutine completes.
```cpp
IAsyncOperationWithProgress<BakingResult, double> BakeMuffinAsync()
{
auto progress = co_await get_progress_token();
BakingResult result;
result.Status(BakingStatus::WarmingUp);
progress.set_result(result);
progress(0.0);
result.Status(BakingStatus::Baking);
progress(0.2);
result.Muffin(Muffin());
result.Status(BakingStatus::Success);
progress(1.0);
co_return result;
}
```
Since clients of ...WithProgress are permitted to call GetResults()
prior to completion, this introduces a few new wrinkles.
1. We need to use the lock to ensure a client doesn't try to
read a partial result at the same time we are writing a new one.
2. The GetResults() method must return a copy of the result.
This is true even if the coroutine has completed, because
a progress handler might go off and do some asynchronous work,
and then come back and ask for the intermediate result. If the
GetResults() method moves the results of a completed coroutine,
then the async progress handler and the co_await will compete
to retrieve the completed results, and somebody will lose.
As a result, ...WithProgress coroutines are slightly more expensive
than non-progress coroutines due to the extra locking and copying.
Fortunately, it's mostly pay-for-play. If a coroutine never calls
`set_result()`, and the client never calls `GetResults()` from its
progress handler, then there is no new lock contention. The final
copy is unavoidable, but fortunately, the result is usually a reference
type, so you just pay an extra AddRef/Release pair.
kennykerr
approved these changes
Mar 5, 2021
Collaborator
kennykerr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - thanks!
jonwis
approved these changes
Mar 5, 2021
Every call to `atomic.load()` goes to memory. Cache the value to remove extra memory accesses. Split more of the implementation of `GetResult()` since there are optimizations available to the ...WithProgress cases, since the `illegal_method_call` case can never happen.
This was referenced Mar 15, 2021
This was referenced Mar 15, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Clients of IAsync...WithProgress are permitted to call GetResults() before the operation has completed, allowing them to observe partial results. This behavior is necessary because the Progress type must be a value type, but the operation may want to report reference types in their progress reports. The way to do that is to report the reference type as the GetResults().
The coroutine itself can use the
set_result()method on the progress token to report a result prior to completion, and can set a new intermediate result as often as desired prior to the completion of the coroutine. The value passed toco_returnacts as the final result of the coroutine. Ifset_result()is never called, then the intermediate result is an empty result (null for reference types, default-initialized for value types).In practice, the result of the coroutine is usually a reference type, and the common pattern for coroutines that want to report a reference type as progress is to create the result object, pass it to
set_result()at the start of the coroutine, update the result object as the coroutine progresses, and (redundantly) pass the same result object toco_returnwhen the coroutine completes.Since clients of ...WithProgress are permitted to call GetResults() prior to completion, this introduces a few new wrinkles.
As a result, ...WithProgress coroutines are slightly more expensive than non-progress coroutines due to the extra locking and copying. Fortunately, it's mostly pay-for-play. If a coroutine never calls
set_result(), and the client never callsGetResults()from its progress handler, then there is no new lock contention. The final copy is unavoidable, but fortunately, the result is usually a reference type, so you just pay an extra AddRef/Release pair.