Add forward iterator to async_for_each #16676
Conversation
async_for_each originally assumed random access iterators, but we can also support forward iterators at the cost of some complexity. The main limitation with forward iterators is that we cannot do arithmetic on the iterators to determine the size of the range and to pre-calculate end iterators. So we use counted loops instead when those iterators are used (we dispatch to a separate path for random access iterators).
travisdowns
force-pushed
the
td-async-for-each
branch
from
9c4294a
to
c667749
Compare
| if (counter.count >= Traits::interval) { | ||
| co_await ss::coroutine::maybe_yield(); | ||
| counter.count = 0; | ||
| Traits::yield_called(); | ||
| } | ||
| } while (begin != end); | ||
|
|
||
| counter.count += FIXED_COST; |
There was a problem hiding this comment.
So this was only really needed in the fast case?
There was a problem hiding this comment.
We do it both cases both before and after this change. The old logic was:
fast() {
if (fast case applies) {
do_fast_case
coutner += .... + FIXED_COST
return;
}
return slow_case();
}
So we needed the increment in slow case since we do the fast case only if it applies.
New logic is like:
fast() {
do_fast_case(... first chunk ...)
coutner += .... + FIXED_COST
if (fast_case did the whole thing) {
return;
}
return slow_case();
}
That is, we do the fast case unconditionally and then check if it was enough. So FIXED_COST is already always added before slow_case executes so we do not need to do it again here.
That said, handling this "right" in the slow case doesn't matter much because FIXED_COST = 1 and so only makes a meaningful difference for small inputs (especially length=0) which would be handled by the fast case. So if it's useful we can be a bit sloppy in the math. The main to avoid is that processing a very long run of zero-length inputs does yield now and then.
There was a problem hiding this comment.
New logic is about 20% slower though for small cases, not sure what's up with that.
|
|
||
| // using container_types = ::testing::Types<std::vector<int>>; | ||
| using container_types | ||
| = ::testing::Types<std::vector<int>, std::unordered_map<int, int>>; |
There was a problem hiding this comment.
Make all tests except one execute for both vector and unordered_map (latter of which has only a forward iterator).
|
|
||
| task_counter t_counter; | ||
| ssx::async_for_each_counter<test_traits<3>>( | ||
| a_counter, v.begin(), v.end(), add_one) | ||
| ssx::async_for_each_counter<test_traits<4>>( |
There was a problem hiding this comment.
Some minor math changes, related to exactly if we yield this time or the next when we are right at a boundary. I don't think it matters (people should not rely on the exact yielding semantics which are timing dependent anyway).
|
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45253#018dd2a4-8531-4b04-8f11-e8f5d8522e25 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45253#018dd2b6-7040-4d2a-b692-521ffd5a7d49 |
|
/backport v23.3.x |
|
/backport v23.2.x |
|
Failed to create a backport PR to v23.3.x branch. I tried: |
This comment was marked as outdated.
This comment was marked as outdated.
|
/backport v23.3.x |
|
/backport v23.2.x |
This comment was marked as outdated.
This comment was marked as outdated.
async_for_each originally assumed random access iterators, but we can also support forward iterators at the cost of some complexity.
The main limitation with forward iterators is that we cannot do arithmetic on the iterators to determine the size of the range and to pre-calculate end iterators. So we use counted loops instead when those iterators are used (we dispatch to a separate path for random access iterators).
Backports Required
Release Notes