-
Notifications
You must be signed in to change notification settings - Fork 15
Add example with coroutines #24
base: master
Are you sure you want to change the base?
Conversation
7062eb2
to
bd2445a
Compare
There is still one problem - the example will sometimes run forever. It seems to sometimes hang on mutex inside runtime_sleep() - probably a deadlock. From what I see there is one other thread that is holding this lock within runtime_waker_wake() (called from vdm_memcpy_cb). Is this some bug in my code? |
ef7e7c1
to
d201a1d
Compare
It might be a bug with the current runtime waker implementation - I probably spent less than 5 minutes thinking about it. But yes, I think using the waker mechanism for this purpose makes much more sense - or maybe we need to implement some other notifier/resume mechanism in the low-level API? |
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.
Reviewed 2 of 7 files at r7.
Reviewable status: 2 of 12 files reviewed, all discussions resolved (waiting on @lukaszstolarczuk)
examples/basic_cpp/basic.cpp, line 109 at r7 (raw file):
} task run_async_mempcy(char *dst, char *src, size_t n)
FYI, misspell mempcy
79d0c4f
to
2c74290
Compare
46006c7
to
6c8aec8
Compare
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.
Reviewed 17 of 20 files at r8, all commit messages.
Reviewable status: 17 of 20 files reviewed, 11 unresolved discussions (waiting on @igchor and @lukaszstolarczuk)
-- commits, line 6 at r8:
you can certainly put this info into the code
-- commits, line 9 at r8:
can know (?)
-- commits, line 13 at r8:
and and
CMakeLists.txt, line 45 at r8 (raw file):
include(${CMAKE_SOURCE_DIR}/cmake/functions.cmake) if(BUILD_CPP_EXAMPLES AND "${CMAKE_CXX_STANDARD}" LESS 20)
you can move this into if (BUILD_EXAMPLES)
at the bottom of this file
.github/workflows/on_pull_request.yml, line 29 at r8 (raw file):
CONFIG: ["N=1 OS=ubuntu OS_VER=21.04 TYPE=normal CC=gcc COVERAGE=1", "N=2 OS=ubuntu OS_VER=21.04 TYPE=normal CC=clang PUSH_IMAGE=1", "N=3 OS=fedora OS_VER=35 TYPE=normal CC=gcc PUSH_IMAGE=1 CXX_STANDARD=20 BUILD_CPP_EXAMPLES=1"]
CXX_STANDARD is set by default to 20 in build.sh, so it seems redundant here
examples/coroutine_memcpy/coroutine_helpers.hpp, line 3 at r8 (raw file):
/* SPDX-License-Identifier: BSD-3-Clause */ /* Copyright 2021-2022, Intel Corporation */ // SPDX-License-Identifier: MIT
I believe you should also update the LICENSE file, then
examples/coroutine_memcpy/coroutine_memcpy.cpp, line 21 at r8 (raw file):
#include "executor.hpp" #include "miniasync_operation.hpp"
you could add some comments, at least in this file 😉
examples/coroutine_memcpy/coroutine_memcpy.cpp, line 31 at r8 (raw file):
auto a1 = async_memcpy(executor, dst, src, 1); auto a2 = async_memcpy(executor, dst + 1, src, 1);
just wondering - did you mean src + 1
(and +2
below)?
examples/coroutine_memcpy/coroutine_memcpy.cpp, line 65 at r8 (raw file):
executor.run_to_completion(); std::cout << buffer << std::endl;
you could add here some << "Final buffer content: "
or something
examples/coroutine_memcpy/miniasync_operation.hpp, line 5 at r8 (raw file):
/* * miniasync_operation.hpp - awaitable wrapper around miniasync operations
you could write here why do you need that
utils/docker/run-build.sh, line 19 at r8 (raw file):
TEST_DIR=${LIBMINIASYNC_TEST_DIR:-${DEFAULT_TEST_DIR}} EXAMPLE_TEST_DIR="/tmp/libminiasync_example_build"
don't you need to set up here some defaults (as well)
How it works: When co_await async_memcpy is called, the compiler calls async_memcpy::awaitable::await_suspend() which puts miniasync future and coroutine handle to a shared containers. It also registers miniasync notifier so that other entities can now when the future completes. There is also an executor loop which takes elements from the container and executes them (calls runtime_wait for miniasync future and and then resumes the coroutine).
It requires C++20 support (-DCMAKE_CXX_STANDARD=20) and a relatively new compiler.
coroutine_memcpy.cpp is the main part of this example:
async_memcpy_print and async_memcpy functions are what user would have to write to take advantage of concurrency.
executor.hpp: executes miniasync operations and coroutines.
coroutine_helpers.hpp is an implementation of generic couroutine related functions.
miniaync_operation.hpp is a coroutine wrapper around miniasync futures.
How it works:
When co_await async_memcpy is called, the compiler calls
async_memcpy::awaitable::await_suspend() which puts miniasync
future and coroutine handle to shared containers.
There is also an executor loop that takes elements from that containers
and executes them (calls runtime_wait for miniasync future and
after that, resumes the coroutine). This is kind of similar to an example here: https://rust-lang.github.io/async-book/02_execution/04_executor.html
I also had one alternative design in mind: instead of having a queue of futures we could just use waker capabilities. Specifically, in
memcpy_task::await_suspend
we could just callfuture_poll
and pass a waker which would resume the coroutine (h
handle). But in this design, we lose the ability to control when coroutines are resumed (right now we can control it in the wait() method).@pbalcer what do you think? would the waker approach even work (I assume the waker would be woken only once - after operation complete? at least for memcpy/memset)?
This change is