Extend collect any #371

qicosmos · 2024-03-08T07:48:15Z

Why

extend collectAny with callback, similar with go/kotlin select coroutine function.

What is changing

add two new collectAny methods

Example

collectAny with variadic callback functions:

    auto test0 = []() -> Lazy<Unit> { co_return Unit{}; };
    auto test1 = []() -> Lazy<int> { co_return 42; };
    auto test2 = [](int val) -> Lazy<std::string> {
        co_return std::to_string(val);
    };

    int call_count = 0;
    index =
        syncAwait(collectAny(std::pair{test0(), [&](auto) { call_count++; }},
                             std::pair{test1(),
                                       [&](auto val) {
                                           call_count++;
                                           EXPECT_EQ(val.value(), 42);
                                       }},
                             std::pair{test2(42), [&](auto val) {
                                           call_count++;
                                           EXPECT_EQ("42", val.value());
                                       }}));
    EXPECT_EQ(1, call_count);

collectAny with vector callback function:

    auto test0 = []() -> Lazy<int> { co_return 41; };
    auto test1 = []() -> Lazy<int> { co_return 42; };

    std::vector<Lazy<int>> input;
    input.push_back(test0());
    input.push_back(test1());

    syncAwait(collectAny(std::move(input), [](size_t index, Try<int> val) {
        if (index == 0) {
            EXPECT_EQ(val.value(), 41);
        } else {
            EXPECT_EQ(val.value(), 42);
        }
    }));

ChuanqiXu9 · 2024-03-11T02:48:19Z

async_simple/coro/Collect.h

+    async_simple::coro::Lazy<size_t> operator()(auto&&... lazy) {
+        auto result = co_await collectAny(std::move(lazy)...);
+
+        tupleSwitch(result.index(), _callback_tuple, result);
+        co_return result.index();


Let's try to avoid adding layers in implementing fundamental functions by new co_await. It may results new allocations/deallocations, which may be a performance concern.

ChuanqiXu9 · 2024-03-11T02:50:06Z

async_simple/coro/test/LazyTest.cpp

+    index = syncAwait(collectAny(std::pair{test3().via(&e1),
+                                           [&](auto val) {
+                                               test_value = val.value();
+                                               EXPECT_EQ(42, test_value);
+                                           }},
+                                 std::pair{test4(41).via(&e1), [&](auto val) {
+                                               test_value = val.value();
+                                               EXPECT_EQ(41, test_value);
+                                           }}));


It will be better if we can avoid writing std::pair and try to use {, }. It may be possible to construct a pair from the initializer list.

I think maybe it's impossible: https://stackoverflow.com/questions/65071836/template-argument-deduction-for-implicit-pair

can't use std::initializer_list to avoid std::pair with brace initialize.

another reason, can't use std::initializer_list because it requires types are the same, not variadic template.

ChuanqiXu9

Also we need to add documents for this.

ChuanqiXu9 · 2024-03-12T06:02:29Z

async_simple/coro/Collect.h

+            std::make_tuple(std::move(std::get<1>(std::get<I>(tuple)))...));
+    }(std::make_index_sequence<sizeof...(Ts)>());
+
+    auto result = co_await std::move(lazy_pair.first);


This still introduces new coroutines, which may be a performance concern. Let's try to avoid the new creation of coroutines as much as possible.

ChuanqiXu9 · 2024-03-12T06:05:07Z

async_simple/coro/test/LazyTest.cpp

+    index = syncAwait(collectAny(std::pair{test3().via(&e1),
+                                           [&](auto val) {
+                                               test_value = val.value();
+                                               EXPECT_EQ(42, test_value);
+                                           }},
+                                 std::pair{test4(41).via(&e1), [&](auto val) {
+                                               test_value = val.value();
+                                               EXPECT_EQ(41, test_value);
+                                           }}));


ChuanqiXu9

We also need to update the document for such new features.

ChuanqiXu9 · 2024-03-13T08:45:09Z

.clang-format

+Language: ObjC
+DisableFormat: true


This should be related.

Please remove the unrelated change.

to solve this error:https://github.com/alibaba/async_simple/actions/runs/8200305198/job/22426819367

This must not be correct. There is nothing related to ObjC.

ChuanqiXu9 · 2024-03-13T08:48:58Z

async_simple/coro/Collect.h

-    return detail::collectAnyImpl(std::move(input));
+          typename IAlloc = std::allocator<LazyType<T>>, typename... Function>
+inline auto collectAny(std::vector<LazyType<T>, IAlloc>&& input,
+                       Function... func) {


Why the Function is variadic? There should be only one function with the vector version.

to reuse code, the original vector version no callback function, if user don't pass a callback function, the code go back to original version, otherwise go to callback version

I feel better with offering an overload here to make things explciitly.

OK, remove the variadic template, use a template parameter: Callback, the default Callback type is Unit, and the
default Callback member variable can be optimized by compiler via [[no_unique_address]].

ChuanqiXu9 · 2024-03-13T08:49:28Z

async_simple/coro/Collect.h

@@ -83,18 +83,22 @@ struct CollectAnyResult {
 #endif
 };

-template <typename LazyType, typename InAlloc>
+template <typename LazyType, typename InAlloc, typename... Function>


Suggested change

template <typename LazyType, typename InAlloc, typename... Function>

template <typename LazyType, typename InAlloc, typename... Callback>

Let's rename Function to Callback. Ditto for the following patch.

async_simple/coro/Collect.h

ChuanqiXu9 · 2024-03-13T09:11:57Z

async_simple/coro/Collect.h

+
+    auto await_resume() {
+        assert(_result != nullptr);
+        return std::move(_result->value());


Why we don't return value with callbacks for vectors but returning here?

variadic version no need index, because the sequence is the index. the index for vector version.

vector version return index is good, unify the return type of vector and variadic version is better.

ChuanqiXu9

We need to document this

ChuanqiXu9 · 2024-03-14T03:42:23Z

.clang-format

+Language: ObjC
+DisableFormat: true


Please remove the unrelated change.

ChuanqiXu9 · 2024-03-14T03:42:48Z

async_simple/coro/test/LazyTest.cpp

-        EXPECT_STREQ("testCollectAllVariadic", v_try_string.value().c_str());
-
-        CHECK_EXECUTOR(&e1);
+// TEST_F(LazyTest, testCollectAllVariadic) {


Why the tests got commented out?

just for test, will revert.

ChuanqiXu9 · 2024-03-14T03:42:57Z

async_simple/coro/test/CMakeLists.txt

@@ -1,4 +1,5 @@
 file(GLOB coro_test_src "*.cpp")
+add_compile_options("$<$<CXX_COMPILER_ID:MSVC>:/bigobj>")


Why do we need this?

windows ci failed before,
https://github.com/alibaba/async_simple/actions/runs/8200305183/job/22426819820#step:4:304

This shouldn't be related to this patch. Please remove the unrelated change.

OK, will create a new PR to add this compile option for msvc.

done in this PR: #375

ChuanqiXu9 · 2024-03-14T08:38:05Z

.github/workflows/conan.yml

+      - name: Install newer Clang
+        run: |
+          wget https://apt.llvm.org/llvm.sh
+          chmod +x ./llvm.sh
+          sudo ./llvm.sh 17


We should do one thing in one patch. Please move this to other PR.

OK, when test passed will create a new pr to upgrade clang for conan.

This PR upgrade clang version: #373

ChuanqiXu9 reviewed Mar 11, 2024

View reviewed changes

ChuanqiXu9 reviewed Mar 12, 2024

View reviewed changes

ChuanqiXu9 reviewed Mar 13, 2024

View reviewed changes

ChuanqiXu9 reviewed Mar 14, 2024

View reviewed changes

qicosmos force-pushed the extend_collectAny branch from b29aaf0 to 5aaeab6 Compare March 14, 2024 09:28

qicosmos added 20 commits March 15, 2024 11:09

support variadic lazy pair for collectAny

163267a

support vector lazy callback for collectAny

8c0017a

update UT

014dbc7

simplify code; add compile option for msvc

6cbafd3

fix clang-format

c50e974

add VariadicPairAwaiter

31e171f

add callback for collectAny vector

6a9bdd9

remove duplicate code

acdcbea

try to fix

212ddff

for msvc compile

4551907

return index for vector version

eb92926

for test

fe58510

rename add overload

37e3266

remove variadic

b5b305a

simplify

2224581

avoid copy

85cbdaa

revert test code and format config

ba7a6b3

update doc of collectAny

fad6f47

update clang

976d368

remove install gtest

3de9c8e

qicosmos force-pushed the extend_collectAny branch from 5aaeab6 to 3de9c8e Compare March 15, 2024 03:10

qicosmos merged commit 6be48e7 into alibaba:main Mar 15, 2024
14 checks passed

qicosmos deleted the extend_collectAny branch March 15, 2024 05:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend collect any #371

Extend collect any #371

qicosmos commented Mar 8, 2024

ChuanqiXu9 Mar 11, 2024

ChuanqiXu9 Mar 11, 2024

qicosmos Mar 12, 2024

ChuanqiXu9 Mar 12, 2024

ChuanqiXu9 left a comment

ChuanqiXu9 Mar 12, 2024

ChuanqiXu9 Mar 12, 2024

ChuanqiXu9 left a comment

ChuanqiXu9 Mar 13, 2024

ChuanqiXu9 Mar 14, 2024

qicosmos Mar 14, 2024

ChuanqiXu9 Mar 14, 2024

ChuanqiXu9 Mar 13, 2024

qicosmos Mar 13, 2024 •

edited

Loading

ChuanqiXu9 Mar 14, 2024

qicosmos Mar 14, 2024

ChuanqiXu9 Mar 13, 2024

ChuanqiXu9 Mar 13, 2024

qicosmos Mar 13, 2024

qicosmos Mar 13, 2024 •

edited

Loading

ChuanqiXu9 left a comment

ChuanqiXu9 Mar 14, 2024

ChuanqiXu9 Mar 14, 2024

qicosmos Mar 14, 2024

ChuanqiXu9 Mar 14, 2024

qicosmos Mar 14, 2024

ChuanqiXu9 Mar 14, 2024

qicosmos Mar 15, 2024

qicosmos Mar 15, 2024

ChuanqiXu9 Mar 14, 2024

qicosmos Mar 14, 2024

qicosmos Mar 14, 2024

	template <typename LazyType, typename InAlloc, typename... Function>
	template <typename LazyType, typename InAlloc, typename... Callback>

		@@ -1,4 +1,5 @@
		file(GLOB coro_test_src "*.cpp")
		add_compile_options("$<$<CXX_COMPILER_ID:MSVC>:/bigobj>")

		Language: ObjC
		DisableFormat: true

		Language: ObjC
		DisableFormat: true

Extend collect any #371

Extend collect any #371

Conversation

qicosmos commented Mar 8, 2024

Why

What is changing

Example

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChuanqiXu9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChuanqiXu9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qicosmos Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qicosmos Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

ChuanqiXu9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qicosmos Mar 13, 2024 •

edited

Loading

qicosmos Mar 13, 2024 •

edited

Loading