[Copilot] Add wil::batched_range and wil::already_complete / already_failed C++/WinRT helpers - #669
Conversation
Implements the two helpers spun out of microsoft/cppwinrt#1608 into WIL: - make_ready() / make_ready(value) / make_failed(): already-settled IAsyncAction / IAsyncOperation<T> with no coroutine frame, firing Completed inline with a single-assignment guard (microsoft#663). - batched(collection): range-for adapter that prefetches elements in blocks via GetMany instead of one ABI crossing per element, for indexed (IVector/IVectorView) and iterable-only (IIterable/IIterator, including map IKeyValuePair) collections (microsoft#664). Both live in cppwinrt_helpers.h next to to_vector, reusing its is_winrt_vector_like / is_winrt_iterator_like detection and the re-includable per-header guard pattern. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Collapse batched_indexed_iterator and batched_buffered_iterator into a single input iterator parameterized on a small refill 'source' policy. As a range-for-only adapter it no longer needs Size(), the GetAt fallback, or random access: it block-prefetches via GetMany and stops when a block comes back short, exactly matching to_vector's exhaustion rule. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Rename the public helper and its detail range struct (batched_range -> batched_view) to avoid a name clash. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Cover exact block-boundary multiples (1/127/128/129/256/257) on both the indexed and iterable paths to exercise the full-block-then-empty-refill termination, ordering across seams, single element, and an IIterator advanced past its start yielding only the remainder. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The failed-operation path never reads m_result (GetResults throws first), but default-constructing it would activate a projected runtimeclass result -- or fail to compile for a class without a default constructor (e.g. Uri). Init the storage with a null handle for object types and a value-init otherwise. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Duncan Horn (dunhor)
left a comment
There was a problem hiding this comment.
Primary concern I have is that the iterator type holds the array storage, which is alarming for two reasons:
- The end iterator also holds this storage, since it is the same type, even though it'll never be used
- Iterators in the STL and STL-like algorithms pass iterators by value most of the time as they are assumed to be cheap to copy. In the worst case, a single algorithm function may delegate to other functions, passing iterators by value, in a loop.
(1) should be addressable by changing to a sentinel type for the end iterator, which should at least be okay in C++20 and beyond b/c of ranges. I did a quick test and it seemed to also work when targeting C++17.
(2) is more alarming, however maybe we just say that this type should only ever be used for loops. Personally, I'm not super thrilled with that limitation, but should we choose to go that route, we may want to at least delete the copy constructor and remove all of the iterator_traits types so that STL algorithms will reject the call.
Alternatively, it might be worth sticking the storage into the range adapter type and just have the iterators reference back to that object instead.
| } | ||
| } | ||
|
|
||
| // Number of elements to prefetch per GetMany call. Aim for ~2KB blocks, clamped to [1, 128]. |
There was a problem hiding this comment.
Curious about the choice of 128 since it' not documented here. Is it just "collections are rarely this big"?
The comment says this "aims" for 2 KiB blocks, however 128 pointers - which is probably the typical case - is 1 KiB
| } | ||
|
|
||
| Source m_source{}; | ||
| std::array<value_type, buffer_capacity> m_buffer{}; |
There was a problem hiding this comment.
So, the end iterator also holds a potentially large array? 😬. One possible option would be to use a separate sentinel type, however these types would no longer be usable with the old STL algorithms (but would work with the ranges library).
Similarly, this would also be problematic when calling STL or STL-like algorithms, which take iterator arguments by value as iterators are typically assumed to be cheap to copy.
| struct batched_iterator | ||
| { | ||
| using value_type = typename Source::value_type; | ||
| using iterator_category = std::input_iterator_tag; |
There was a problem hiding this comment.
Input iterators require *it++ to be well formed and be equivalent to:
auto __val = *it;
++it;This type only provides pre-increment and therefore can potentially fail to compile, e.g. when calling STL algorithms. Adding pre-increment is probably not the right call since a copy of this iterator type is such a heavy operation (another property that's very unusual for iterators).
|
|
||
| reference operator*() const noexcept | ||
| { | ||
| return m_buffer[m_index]; |
There was a problem hiding this comment.
Some asserts would be nice. E.g. WI_ASSERT(m_index < m_size)
|
|
||
| bool operator==(batched_iterator const& other) const noexcept | ||
| { | ||
| return (m_size == 0) && (other.m_size == 0); |
There was a problem hiding this comment.
This would break self-equality (i.e. a == a) for non-end iterators. Probably never an issue in practice (realistically someone would be comparing copies) especially given that this is an input iterator, but is enough to cause a feeling of uneasiness.
|
|
||
| void Completed(CompletedHandler const& handler) | ||
| { | ||
| // Match the coroutine promise contract: Completed may be assigned at most once. |
There was a problem hiding this comment.
nit: this is a WinRT contract, not a coroutine contract
|
|
||
| if (handler) | ||
| { | ||
| handler(static_cast<Derived*>(this)->get_strong().template as<AsyncInterface>(), Status()); |
There was a problem hiding this comment.
It's been a while since I used C++/WinRT extensively, so this might be wrong, but this appears to be:
- Performing an
AddRefon the derived object, and then - Performing a
QueryInterfaceon the derived object
I can see an argument for increasing the reference count once (the caller may free its only reference in the callback), but two sems unnecessary.
| void Completed(CompletedHandler const& handler) | ||
| { | ||
| // Match the coroutine promise contract: Completed may be assigned at most once. | ||
| if (std::exchange(m_completed_assigned, true)) |
There was a problem hiding this comment.
std::exchange is not an atomic operation. In practice that's probably fine, especially since this type doesn't store the object, but another place that gives the feeling of uneasiness.
| // value-init. Mirrors wil::details::empty<T>, redefined here because that lives under the | ||
| // Collections guard while this block only requires Windows.Foundation. |
There was a problem hiding this comment.
Perhaps invert the two? Define empty<T> here and have the collections code use this definition.
|
|
||
| template <typename TResult> | ||
| struct ready_async_operation | ||
| : ready_async_base<ready_async_operation<TResult>, winrt::Windows::Foundation::IAsyncOperation<TResult>, winrt::Windows::Foundation::AsyncOperationCompletedHandler<TResult>> |
There was a problem hiding this comment.
Maybe I'm just being dumb, but is there a reason these derive from the completed handler delegate types?
|
FWIW (2) is why I didn't like the idea of making it the default for cppwinrt as was initially intended, and instead wanted to port it to a separate type. |
What this adds
Two C++/WinRT helpers in
wil/cppwinrt_helpers.h, spun out of the review discussion on microsoft/cppwinrt#1608 — C++/WinRT stays projection-only, so these interop conveniences live in WIL. Resolves #663 and #664.wil::already_complete/wil::already_failed(#663)Return an
IAsyncAction/IAsyncOperation<T>that is already settled, with no coroutine frame —co_await,.get(), and aCompletedhandler all complete synchronously.A single
winrt::implementsobject over the async interface +IAsyncInfo;Completedis fired inline with a single-assignment guard (throwshresult_illegal_delegate_assignmenton a second set), no mutex — the compact shape from Raymond Chen's "already-completed asynchronous activity" series. The result storage is null-initialized for projected object types so the faulted path never activates (or requires a default constructor for) a runtimeclass result.wil::batched_range(#664)A range-for adapter that prefetches elements in blocks via
GetManyinstead of one ABI round-trip per element. On a cross-process or heavily-marshaled collection the per-element crossings dominate, so batching cuts them to roughly one per block.Works for
IVector<T>,IVectorView<T>,IIterable<T>,IIterator<T>, and anything C++/WinRT projects those for (PropertySet,IMap<K,V>, …). Indexed collections prefetch withGetMany(index, …); iterable-only collections buffer throughIIterator::GetMany. It's a single-pass input range — a yielded element outlives the step that produced it, matching the observable behavior ofwil::to_vector. Block prefetch stops onceGetManyreturns a short block. Both shapes share one input iterator that differs only in how a block is refilled.Testing
New coverage in
tests/CppWinRTTests.cpp:already_complete/already_failedcompleted/faulted status,co_awaitand.get(), inlineCompletedfiring, double-assignment throwing, a runtimeclass (Uri) result on the faulted path, and non-trivial result round-trip;batched_rangeacross the indexed and iterable paths, map/IKeyValuePair, exact block-boundary multiples (1/127/128/129/256/257) for ordering and clean termination, empty collections, duck-typed non-WinRT shapes, and an iterator advanced past its start yielding only the remainder. Built clean across clang and MSVC (debug + relwithdebinfo); full[cppwinrt]suite passes.References
(via Copilot)