asio strand scheduler#627
Conversation
WalkthroughWalkthroughThe changes introduce a new build configuration option for the RPP library, specifically the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CMake
participant RPP
participant ASIO
User->>CMake: Configure build with RPP_BUILD_ASIO_CODE ON
CMake->>RPP: Include ASIO dependencies
RPP->>ASIO: Link ASIO library
RPP->>User: Build successful with ASIO support
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
BUILDING.md (1)
31-31: Fix the grammatical issue.The description contains a grammatical issue. Apply this diff to fix it:
- `RPP_BUILD_ASIO_CODE` - (ON/OFF) build RPPASIO related code (examples/tests)(rppasio module doesn't requires this one) (default OFF) - requires asio to be installed + `RPP_BUILD_ASIO_CODE` - (ON/OFF) build RPPASIO related code (examples/tests)(rppasio module doesn't require this one) (default OFF) - requires asio to be installedTools
LanguageTool
[grammar] ~31-~31: The auxiliary verb ‘do’ requires the base form of the verb.
Context: ...(examples/tests)(rppasio module doesn't requires this one) (default OFF) - requires asio...(DID_BASEFORM)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (16)
- BUILDING.md (1 hunks)
- CMakePresets.json (3 hunks)
- cmake/dependencies.cmake (1 hunks)
- cmake/install-rules.cmake (1 hunks)
- cmake/variables.cmake (2 hunks)
- conanfile.py (2 hunks)
- src/extensions/CMakeLists.txt (1 hunks)
- src/extensions/rppasio/CMakeLists.txt (1 hunks)
- src/extensions/rppasio/rppasio/fwd.hpp (1 hunks)
- src/extensions/rppasio/rppasio/rppasio.hpp (1 hunks)
- src/extensions/rppasio/rppasio/schedulers.hpp (1 hunks)
- src/extensions/rppasio/rppasio/schedulers/fwd.hpp (1 hunks)
- src/extensions/rppasio/rppasio/schedulers/strand.hpp (1 hunks)
- src/tests/CMakeLists.txt (2 hunks)
- src/tests/rppasio/test_rppasio_scheduler.cpp (1 hunks)
- src/tests/utils/rpp_trompeloil.hpp (2 hunks)
Files skipped from review due to trivial changes (5)
- src/extensions/rppasio/CMakeLists.txt
- src/extensions/rppasio/rppasio/fwd.hpp
- src/extensions/rppasio/rppasio/rppasio.hpp
- src/extensions/rppasio/rppasio/schedulers.hpp
- src/extensions/rppasio/rppasio/schedulers/fwd.hpp
Additional context used
LanguageTool
BUILDING.md
[grammar] ~31-~31: The auxiliary verb ‘do’ requires the base form of the verb.
Context: ...(examples/tests)(rppasio module doesn't requires this one) (default OFF) - requires asio...(DID_BASEFORM)
Additional comments not posted (31)
src/extensions/CMakeLists.txt (1)
3-3: LGTM!The addition of the
rppasiosubdirectory is consistent with the existing structure.conanfile.py (3)
14-15: LGTM!The addition of the
with_asiooption is consistent with the existing options.
22-23: LGTM!The addition of the
with_asiodefault option is consistent with the existing default options.
42-44: LGTM! But verify the usage of the new option.The conditional inclusion of the
asiodependency is correctly implemented. However, ensure that all necessary parts of the codebase handle the newwith_asiooption appropriately.Run the following script to verify the usage of the new option:
cmake/install-rules.cmake (1)
35-39: LGTM!The addition of the installation rule for
rppasiois consistent with the existing rules for other targets.src/tests/CMakeLists.txt (2)
31-33: LGTM!The conditional logic for
rppasiosupport is correctly implemented.
67-69: LGTM!The conditional logic for
rppasiotest registration is correctly implemented.src/tests/utils/rpp_trompeloil.hpp (2)
8-9: LGTM!The inclusion of the
rpp/observers/observer.hppheader is necessary for the new member functions in themock_observerclass.
71-73: LGTM!The new member functions enhance the
mock_observerclass's functionality by providing flexible ways to obtain observers.cmake/variables.cmake (2)
86-86: LGTM!The addition of the new configuration option
RPP_BUILD_ASIO_CODEis straightforward and follows the existing pattern for other options.
108-110: LGTM!The conditional block that appends ASIO-related arguments to the
CONAN_ARGSvariable is correctly implemented and follows the existing pattern for other options.src/extensions/rppasio/rppasio/schedulers/strand.hpp (6)
20-69: LGTM!The class
strandand its nested classstateare correctly implemented and follow best practices for ASIO integration.
37-65: LGTM!The
defer_formethod is correctly implemented and handles both immediate and delayed execution of tasks.
71-90: LGTM!The class
worker_strategyis correctly implemented and follows best practices for ASIO integration.
79-83: LGTM!The
defer_formethod is correctly implemented and delegates the task to thestateclass.
98-101: LGTM!The
create_workermethod is correctly implemented and returns a worker for the strand.
92-95: LGTM!The constructor is correctly implemented and initializes the strand with an executor.
src/tests/rppasio/test_rppasio_scheduler.cpp (5)
40-65: LGTM!The test case
strand utilized current_threadis correctly implemented and verifies the expected behavior.
67-83: LGTM!The test case
strand works till endis correctly implemented and verifies the expected behavior.
86-105: LGTM!The test case
strand in combination with current_threadis correctly implemented and verifies the expected behavior.
107-138: LGTM!The test case
strand worker has correct execution orderis correctly implemented and verifies the expected behavior.
31-37: LGTM!The
drainfunction is correctly implemented and ensures that the context runs until the disposable is disposed.cmake/dependencies.cmake (2)
128-131: LGTM!The conditional block for ASIO support is correctly implemented.
132-135: LGTM!The macro
rpp_add_asio_support_to_executableis correctly implemented.CMakePresets.json (7)
148-154: LGTM!The new preset "build-asio" is correctly implemented.
178-178: LGTM!The updated preset "ci-coverage-clang" is correctly implemented.
187-187: LGTM!The updated preset "ci-sanitize-tsan" is correctly implemented.
194-194: LGTM!The updated preset "ci-sanitize-asan" is correctly implemented.
201-201: LGTM!The updated preset "ci-sanitize-lsan" is correctly implemented.
208-208: LGTM!The updated preset "ci-sanitize-msan" is correctly implemented.
215-215: LGTM!The updated preset "ci-sanitize-ubsan" is correctly implemented.
| namespace rppasio::schedulers | ||
| { | ||
| /** | ||
| * @brief TODO |
BENCHMARK RESULTS (AUTOGENERATED)
|
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| Subscribe empty callbacks to empty observable | 305.22 ns | 2.16 ns | 2.16 ns | 1.00 |
| Subscribe empty callbacks to empty observable via pipe operator | 305.26 ns | 2.16 ns | 2.16 ns | 1.00 |
Sources
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| from array of 1 - create + subscribe + immediate | 697.51 ns | 0.31 ns | 0.31 ns | 1.00 |
| from array of 1 - create + subscribe + current_thread | 1038.15 ns | 3.71 ns | 3.42 ns | 1.08 |
| concat_as_source of just(1 immediate) create + subscribe | 2262.74 ns | 114.51 ns | 104.71 ns | 1.09 |
| defer from array of 1 - defer + create + subscribe + immediate | 729.66 ns | 0.31 ns | 0.31 ns | 1.00 |
| interval - interval + take(3) + subscribe + immediate | 2092.99 ns | 59.23 ns | 59.23 ns | 1.00 |
| interval - interval + take(3) + subscribe + current_thread | 3029.53 ns | 32.40 ns | 32.46 ns | 1.00 |
| from array of 1 - create + as_blocking + subscribe + new_thread | 30301.29 ns | 31626.65 ns | 35779.06 ns | 0.88 |
| from array of 1000 - create + as_blocking + subscribe + new_thread | 38147.72 ns | 52609.15 ns | 55631.67 ns | 0.95 |
| concat_as_source of just(1 immediate) and just(1,2 immediate)create + subscribe | 3470.24 ns | 143.69 ns | 127.89 ns | 1.12 |
Filtering Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate_just+take(1)+subscribe | 1082.11 ns | 0.31 ns | 0.31 ns | 1.00 |
| immediate_just+filter(true)+subscribe | 874.02 ns | 0.31 ns | 0.31 ns | 1.00 |
| immediate_just(1,2)+skip(1)+subscribe | 999.58 ns | 0.31 ns | 0.31 ns | 1.00 |
| immediate_just(1,1,2)+distinct_until_changed()+subscribe | 926.82 ns | 0.31 ns | 0.31 ns | 1.00 |
| immediate_just(1,2)+first()+subscribe | 1238.10 ns | 0.31 ns | 0.31 ns | 1.00 |
| immediate_just(1,2)+last()+subscribe | 927.09 ns | 0.31 ns | 0.31 ns | 1.00 |
| immediate_just+take_last(1)+subscribe | 1135.64 ns | 17.90 ns | 18.55 ns | 0.96 |
| immediate_just(1,2,3)+element_at(1)+subscribe | 835.34 ns | 0.31 ns | 0.31 ns | 1.00 |
Schedulers
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate scheduler create worker + schedule | 279.50 ns | 2.16 ns | 2.16 ns | 1.00 |
| current_thread scheduler create worker + schedule | 375.93 ns | 5.86 ns | 5.90 ns | 0.99 |
| current_thread scheduler create worker + schedule + recursive schedule | 814.64 ns | 56.04 ns | 55.62 ns | 1.01 |
Transforming Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate_just+map(v*2)+subscribe | 860.12 ns | 0.31 ns | 0.31 ns | 1.00 |
| immediate_just+scan(10, std::plus)+subscribe | 878.72 ns | 0.31 ns | 0.31 ns | 1.00 |
| immediate_just+flat_map(immediate_just(v*2))+subscribe | 2295.54 ns | 175.40 ns | 162.78 ns | 1.08 |
| immediate_just+buffer(2)+subscribe | 1520.68 ns | 13.58 ns | 14.20 ns | 0.96 |
| immediate_just+window(2)+subscribe + subscsribe inner | 2429.74 ns | 1111.72 ns | 1062.29 ns | 1.05 |
Conditional Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate_just+take_while(false)+subscribe | 918.14 ns | - | - | 0.00 |
| immediate_just+take_while(true)+subscribe | 852.98 ns | 0.31 ns | 0.31 ns | 1.00 |
Utility Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate_just(1)+subscribe_on(immediate)+subscribe | 2022.79 ns | 0.31 ns | 0.31 ns | 1.00 |
Combining Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate_just(immediate_just(1), immediate_just(1)) + merge() + subscribe | 3438.12 ns | 184.57 ns | 187.54 ns | 0.98 |
| immediate_just(1) + merge_with(immediate_just(2)) + subscribe | 3610.80 ns | 178.07 ns | 171.48 ns | 1.04 |
| immediate_just(1) + with_latest_from(immediate_just(2)) + subscribe | - | 145.74 ns | 136.91 ns | 1.06 |
| immediate_just(immediate_just(1),immediate_just(1)) + switch_on_next() + subscribe | 3516.69 ns | 941.03 ns | 885.27 ns | 1.06 |
| immediate_just(1) + zip(immediate_just(2)) + subscribe | 2159.67 ns | 237.12 ns | 225.23 ns | 1.05 |
Subjects
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| publish_subject with 1 observer - on_next | 34.56 ns | 14.73 ns | 14.61 ns | 1.01 |
| subscribe 100 observers to publish_subject | 199273.83 ns | 15567.74 ns | 15421.60 ns | 1.01 |
| 100 on_next to 100 observers to publish_subject | 32519.95 ns | 17128.84 ns | 20460.62 ns | 0.84 |
Scenarios
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| basic sample | 1436.64 ns | 12.97 ns | 12.65 ns | 1.03 |
| basic sample with immediate scheduler | 1414.32 ns | 5.55 ns | 5.24 ns | 1.06 |
Aggregating Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate_just+reduce(10, std::plus)+subscribe | 936.54 ns | 0.31 ns | 0.31 ns | 1.00 |
Error Handling Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| create(on_next(1), on_error())+on_error_resume_next(immediate_just(2)))+subscribe | 2071.30 ns | 1006.05 ns | 1013.24 ns | 0.99 |
| create(on_error())+retry(1)+subscribe | 593.81 ns | 98.37 ns | 107.89 ns | 0.91 |
ci-macos
General
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| Subscribe empty callbacks to empty observable | 1065.39 ns | 4.30 ns | 3.89 ns | 1.11 |
| Subscribe empty callbacks to empty observable via pipe operator | 1056.42 ns | 4.17 ns | 3.87 ns | 1.08 |
Sources
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| from array of 1 - create + subscribe + immediate | 2231.79 ns | 0.26 ns | 0.23 ns | 1.13 |
| from array of 1 - create + subscribe + current_thread | 2640.63 ns | 38.27 ns | 34.68 ns | 1.10 |
| concat_as_source of just(1 immediate) create + subscribe | 5897.81 ns | 357.40 ns | 337.75 ns | 1.06 |
| defer from array of 1 - defer + create + subscribe + immediate | 1955.91 ns | 0.23 ns | 0.24 ns | 0.99 |
| interval - interval + take(3) + subscribe + immediate | 4914.53 ns | 113.72 ns | 113.98 ns | 1.00 |
| interval - interval + take(3) + subscribe + current_thread | 6821.72 ns | 105.91 ns | 97.30 ns | 1.09 |
| from array of 1 - create + as_blocking + subscribe + new_thread | 105694.45 ns | 102624.44 ns | 94219.18 ns | 1.09 |
| from array of 1000 - create + as_blocking + subscribe + new_thread | 96079.55 ns | 126474.75 ns | 99710.60 ns | 1.27 |
| concat_as_source of just(1 immediate) and just(1,2 immediate)create + subscribe | 8660.16 ns | 390.16 ns | 383.78 ns | 1.02 |
Filtering Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate_just+take(1)+subscribe | 3565.56 ns | 0.27 ns | 0.23 ns | 1.18 |
| immediate_just+filter(true)+subscribe | 2106.46 ns | 0.25 ns | 0.22 ns | 1.11 |
| immediate_just(1,2)+skip(1)+subscribe | 2750.12 ns | 0.23 ns | 0.23 ns | 1.02 |
| immediate_just(1,1,2)+distinct_until_changed()+subscribe | 2066.63 ns | 0.47 ns | 0.46 ns | 1.02 |
| immediate_just(1,2)+first()+subscribe | 3176.51 ns | 0.23 ns | 0.22 ns | 1.05 |
| immediate_just(1,2)+last()+subscribe | 2389.61 ns | 0.23 ns | 0.22 ns | 1.05 |
| immediate_just+take_last(1)+subscribe | 3180.67 ns | 0.23 ns | 0.22 ns | 1.05 |
| immediate_just(1,2,3)+element_at(1)+subscribe | 2123.04 ns | 0.23 ns | 0.23 ns | 1.02 |
Schedulers
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate scheduler create worker + schedule | 963.23 ns | 4.71 ns | 4.14 ns | 1.14 |
| current_thread scheduler create worker + schedule | 1349.29 ns | 43.78 ns | 38.52 ns | 1.14 |
| current_thread scheduler create worker + schedule + recursive schedule | 1999.02 ns | 204.82 ns | 209.26 ns | 0.98 |
Transforming Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate_just+map(v*2)+subscribe | 2098.12 ns | 4.20 ns | 4.33 ns | 0.97 |
| immediate_just+scan(10, std::plus)+subscribe | 2341.41 ns | 0.47 ns | 0.46 ns | 1.02 |
| immediate_just+flat_map(immediate_just(v*2))+subscribe | 5303.99 ns | 402.55 ns | 400.88 ns | 1.00 |
| immediate_just+buffer(2)+subscribe | 2481.87 ns | 64.43 ns | 62.93 ns | 1.02 |
| immediate_just+window(2)+subscribe + subscsribe inner | 6993.02 ns | 2415.02 ns | 2306.79 ns | 1.05 |
Conditional Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate_just+take_while(false)+subscribe | 2102.48 ns | - | - | 0.00 |
| immediate_just+take_while(true)+subscribe | 2097.78 ns | 0.23 ns | 0.23 ns | 1.02 |
Utility Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate_just(1)+subscribe_on(immediate)+subscribe | 4907.37 ns | 5.13 ns | 4.78 ns | 1.07 |
Combining Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate_just(immediate_just(1), immediate_just(1)) + merge() + subscribe | 7345.64 ns | 438.64 ns | 440.60 ns | 1.00 |
| immediate_just(1) + merge_with(immediate_just(2)) + subscribe | 8360.14 ns | 441.96 ns | 439.71 ns | 1.01 |
| immediate_just(1) + with_latest_from(immediate_just(2)) + subscribe | - | 459.58 ns | 459.90 ns | 1.00 |
| immediate_just(immediate_just(1),immediate_just(1)) + switch_on_next() + subscribe | 7993.45 ns | 1909.54 ns | 1911.64 ns | 1.00 |
| immediate_just(1) + zip(immediate_just(2)) + subscribe | 5103.68 ns | 840.14 ns | 828.49 ns | 1.01 |
Subjects
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| publish_subject with 1 observer - on_next | 76.74 ns | 48.90 ns | 48.72 ns | 1.00 |
| subscribe 100 observers to publish_subject | 344197.00 ns | 40828.96 ns | 40678.50 ns | 1.00 |
| 100 on_next to 100 observers to publish_subject | 51332.74 ns | 18785.19 ns | 16590.69 ns | 1.13 |
Scenarios
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| basic sample | 2761.89 ns | 68.95 ns | 69.65 ns | 0.99 |
| basic sample with immediate scheduler | 2784.64 ns | 18.49 ns | 18.74 ns | 0.99 |
Aggregating Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate_just+reduce(10, std::plus)+subscribe | 2396.43 ns | 0.23 ns | 0.23 ns | 1.00 |
Error Handling Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| create(on_next(1), on_error())+on_error_resume_next(immediate_just(2)))+subscribe | 6596.40 ns | 4181.09 ns | 4110.65 ns | 1.02 |
| create(on_error())+retry(1)+subscribe | 1808.91 ns | 287.43 ns | 284.80 ns | 1.01 |
ci-ubuntu-clang
General
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| Subscribe empty callbacks to empty observable | 279.87 ns | 1.54 ns | 1.54 ns | 1.00 |
| Subscribe empty callbacks to empty observable via pipe operator | 267.81 ns | 1.54 ns | 1.54 ns | 1.00 |
Sources
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| from array of 1 - create + subscribe + immediate | 583.72 ns | 0.31 ns | 0.31 ns | 1.00 |
| from array of 1 - create + subscribe + current_thread | 815.39 ns | 4.32 ns | 4.32 ns | 1.00 |
| concat_as_source of just(1 immediate) create + subscribe | 2343.89 ns | 130.96 ns | 130.58 ns | 1.00 |
| defer from array of 1 - defer + create + subscribe + immediate | 789.24 ns | 0.31 ns | 0.31 ns | 1.00 |
| interval - interval + take(3) + subscribe + immediate | 2264.64 ns | 58.31 ns | 58.31 ns | 1.00 |
| interval - interval + take(3) + subscribe + current_thread | 3481.75 ns | 30.88 ns | 30.90 ns | 1.00 |
| from array of 1 - create + as_blocking + subscribe + new_thread | 27098.61 ns | 30819.68 ns | 31702.06 ns | 0.97 |
| from array of 1000 - create + as_blocking + subscribe + new_thread | 38959.44 ns | 38898.88 ns | 36502.28 ns | 1.07 |
| concat_as_source of just(1 immediate) and just(1,2 immediate)create + subscribe | 3732.55 ns | 150.57 ns | 150.14 ns | 1.00 |
Filtering Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate_just+take(1)+subscribe | 1138.46 ns | 0.31 ns | 0.31 ns | 1.00 |
| immediate_just+filter(true)+subscribe | 875.70 ns | 0.31 ns | 0.31 ns | 1.00 |
| immediate_just(1,2)+skip(1)+subscribe | 1087.59 ns | 0.31 ns | 0.31 ns | 1.00 |
| immediate_just(1,1,2)+distinct_until_changed()+subscribe | 920.99 ns | 0.31 ns | 0.31 ns | 1.02 |
| immediate_just(1,2)+first()+subscribe | 1369.48 ns | 0.31 ns | 0.31 ns | 1.00 |
| immediate_just(1,2)+last()+subscribe | 1053.26 ns | 0.31 ns | 0.31 ns | 1.00 |
| immediate_just+take_last(1)+subscribe | 1180.56 ns | 0.31 ns | 0.31 ns | 1.00 |
| immediate_just(1,2,3)+element_at(1)+subscribe | 913.67 ns | 0.31 ns | 0.31 ns | 1.00 |
Schedulers
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate scheduler create worker + schedule | 284.92 ns | 1.54 ns | 1.54 ns | 1.00 |
| current_thread scheduler create worker + schedule | 392.21 ns | 4.94 ns | 4.94 ns | 1.00 |
| current_thread scheduler create worker + schedule + recursive schedule | 905.39 ns | 55.85 ns | 55.73 ns | 1.00 |
Transforming Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate_just+map(v*2)+subscribe | 871.84 ns | 0.31 ns | 0.31 ns | 1.00 |
| immediate_just+scan(10, std::plus)+subscribe | 983.92 ns | 0.31 ns | 0.31 ns | 1.00 |
| immediate_just+flat_map(immediate_just(v*2))+subscribe | 2263.43 ns | 138.43 ns | 138.53 ns | 1.00 |
| immediate_just+buffer(2)+subscribe | 1510.57 ns | 13.60 ns | 13.59 ns | 1.00 |
| immediate_just+window(2)+subscribe + subscsribe inner | 2458.04 ns | 930.03 ns | 928.39 ns | 1.00 |
Conditional Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate_just+take_while(false)+subscribe | 866.77 ns | - | - | 0.00 |
| immediate_just+take_while(true)+subscribe | 868.18 ns | 0.31 ns | 0.31 ns | 1.00 |
Utility Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate_just(1)+subscribe_on(immediate)+subscribe | 2125.59 ns | 0.31 ns | 0.31 ns | 1.00 |
Combining Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate_just(immediate_just(1), immediate_just(1)) + merge() + subscribe | 3275.54 ns | 160.23 ns | 164.77 ns | 0.97 |
| immediate_just(1) + merge_with(immediate_just(2)) + subscribe | 3791.02 ns | 148.05 ns | 151.11 ns | 0.98 |
| immediate_just(1) + with_latest_from(immediate_just(2)) + subscribe | - | 144.34 ns | 143.84 ns | 1.00 |
| immediate_just(immediate_just(1),immediate_just(1)) + switch_on_next() + subscribe | 3389.62 ns | 842.29 ns | 844.08 ns | 1.00 |
| immediate_just(1) + zip(immediate_just(2)) + subscribe | 2246.46 ns | 201.11 ns | 200.48 ns | 1.00 |
Subjects
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| publish_subject with 1 observer - on_next | 53.64 ns | 17.66 ns | 17.55 ns | 1.01 |
| subscribe 100 observers to publish_subject | 213034.25 ns | 15956.58 ns | 15970.75 ns | 1.00 |
| 100 on_next to 100 observers to publish_subject | 42789.80 ns | 20415.21 ns | 20908.68 ns | 0.98 |
Scenarios
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| basic sample | 1323.70 ns | 11.73 ns | 11.73 ns | 1.00 |
| basic sample with immediate scheduler | 1322.12 ns | 6.17 ns | 6.17 ns | 1.00 |
Aggregating Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate_just+reduce(10, std::plus)+subscribe | 1014.31 ns | 0.31 ns | 0.31 ns | 1.00 |
Error Handling Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| create(on_next(1), on_error())+on_error_resume_next(immediate_just(2)))+subscribe | 2177.39 ns | 1233.11 ns | 1272.39 ns | 0.97 |
| create(on_error())+retry(1)+subscribe | 652.56 ns | 138.03 ns | 138.53 ns | 1.00 |
ci-windows
General
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| Subscribe empty callbacks to empty observable | 561.60 ns | 4.02 ns | 4.88 ns | 0.82 |
| Subscribe empty callbacks to empty observable via pipe operator | 589.45 ns | 4.02 ns | 4.94 ns | 0.81 |
Sources
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| from array of 1 - create + subscribe + immediate | 1163.33 ns | 9.71 ns | 9.71 ns | 1.00 |
| from array of 1 - create + subscribe + current_thread | 1425.67 ns | 17.91 ns | 17.60 ns | 1.02 |
| concat_as_source of just(1 immediate) create + subscribe | 3964.04 ns | 189.01 ns | 177.26 ns | 1.07 |
| defer from array of 1 - defer + create + subscribe + immediate | 1459.67 ns | 9.42 ns | 9.41 ns | 1.00 |
| interval - interval + take(3) + subscribe + immediate | 3041.45 ns | 145.32 ns | 144.40 ns | 1.01 |
| interval - interval + take(3) + subscribe + current_thread | 3421.75 ns | 64.29 ns | 65.09 ns | 0.99 |
| from array of 1 - create + as_blocking + subscribe + new_thread | 121000.00 ns | 102272.73 ns | 124000.00 ns | 0.82 |
| from array of 1000 - create + as_blocking + subscribe + new_thread | 135071.43 ns | 166785.71 ns | 146937.50 ns | 1.14 |
| concat_as_source of just(1 immediate) and just(1,2 immediate)create + subscribe | 5263.29 ns | 212.26 ns | 207.82 ns | 1.02 |
Filtering Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate_just+take(1)+subscribe | 1805.82 ns | 25.30 ns | 24.99 ns | 1.01 |
| immediate_just+filter(true)+subscribe | 1308.06 ns | 24.37 ns | 24.05 ns | 1.01 |
| immediate_just(1,2)+skip(1)+subscribe | 1698.73 ns | 24.06 ns | 23.45 ns | 1.03 |
| immediate_just(1,1,2)+distinct_until_changed()+subscribe | 1362.63 ns | 29.01 ns | 26.37 ns | 1.10 |
| immediate_just(1,2)+first()+subscribe | 2054.84 ns | 22.82 ns | 23.74 ns | 0.96 |
| immediate_just(1,2)+last()+subscribe | 1774.73 ns | 24.06 ns | 24.72 ns | 0.97 |
| immediate_just+take_last(1)+subscribe | 1990.04 ns | 69.64 ns | 70.33 ns | 0.99 |
| immediate_just(1,2,3)+element_at(1)+subscribe | 1350.89 ns | 27.47 ns | 26.58 ns | 1.03 |
Schedulers
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate scheduler create worker + schedule | 475.24 ns | 6.17 ns | 6.79 ns | 0.91 |
| current_thread scheduler create worker + schedule | 648.80 ns | 14.21 ns | 14.57 ns | 0.98 |
| current_thread scheduler create worker + schedule + recursive schedule | 1553.58 ns | 108.32 ns | 103.29 ns | 1.05 |
Transforming Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate_just+map(v*2)+subscribe | 1305.37 ns | 24.37 ns | 24.36 ns | 1.00 |
| immediate_just+scan(10, std::plus)+subscribe | 1405.24 ns | 26.53 ns | 26.51 ns | 1.00 |
| immediate_just+flat_map(immediate_just(v*2))+subscribe | 3729.33 ns | 211.52 ns | 212.91 ns | 0.99 |
| immediate_just+buffer(2)+subscribe | 2315.95 ns | 70.47 ns | 69.02 ns | 1.02 |
| immediate_just+window(2)+subscribe + subscsribe inner | 3988.54 ns | 1284.83 ns | 1307.55 ns | 0.98 |
Conditional Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate_just+take_while(false)+subscribe | 1303.55 ns | 23.14 ns | 23.14 ns | 1.00 |
| immediate_just+take_while(true)+subscribe | 1302.17 ns | 24.36 ns | 24.05 ns | 1.01 |
Utility Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate_just(1)+subscribe_on(immediate)+subscribe | 3110.81 ns | 11.11 ns | 11.10 ns | 1.00 |
Combining Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate_just(immediate_just(1), immediate_just(1)) + merge() + subscribe | 5004.93 ns | 225.59 ns | 229.81 ns | 0.98 |
| immediate_just(1) + merge_with(immediate_just(2)) + subscribe | 5649.21 ns | 208.21 ns | 225.48 ns | 0.92 |
| immediate_just(1) + with_latest_from(immediate_just(2)) + subscribe | - | 199.34 ns | 204.26 ns | 0.98 |
| immediate_just(immediate_just(1),immediate_just(1)) + switch_on_next() + subscribe | 5302.78 ns | 918.41 ns | 961.55 ns | 0.96 |
| immediate_just(1) + zip(immediate_just(2)) + subscribe | 3762.83 ns | 525.72 ns | 532.02 ns | 0.99 |
Subjects
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| publish_subject with 1 observer - on_next | 37.36 ns | 20.67 ns | 20.68 ns | 1.00 |
| subscribe 100 observers to publish_subject | 263800.00 ns | 29635.90 ns | 28417.95 ns | 1.04 |
| 100 on_next to 100 observers to publish_subject | 51925.00 ns | 38823.33 ns | 38822.58 ns | 1.00 |
Scenarios
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| basic sample | 1840.92 ns | 102.58 ns | 102.64 ns | 1.00 |
| basic sample with immediate scheduler | 1849.36 ns | 74.75 ns | 74.07 ns | 1.01 |
Aggregating Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| immediate_just+reduce(10, std::plus)+subscribe | 1435.87 ns | 24.99 ns | 24.66 ns | 1.01 |
Error Handling Operators
| name | rxcpp | rpp | prev rpp | ratio |
|---|---|---|---|---|
| create(on_next(1), on_error())+on_error_resume_next(immediate_just(2)))+subscribe | 2371.19 ns | 354.10 ns | 366.82 ns | 0.97 |
| create(on_error())+retry(1)+subscribe | 1172.60 ns | 140.37 ns | 141.04 ns | 1.00 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v2 #627 +/- ##
==========================================
- Coverage 95.69% 95.64% -0.06%
==========================================
Files 97 97
Lines 1882 1883 +1
==========================================
Hits 1801 1801
- Misses 81 82 +1 ☔ View full report in Codecov by Sentry. |
|
@CorentinBT , sonarcloud showing valid issues: you are trying to forward values but they just forwarded inside lambda: from this point it is regular variable and should be just moved. Forward inside lambda is ok in case of capturing by reference |
done |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- BUILDING.md (1 hunks)
- src/extensions/rppasio/rppasio/schedulers/strand.hpp (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- BUILDING.md
Additional comments not posted (4)
src/extensions/rppasio/rppasio/schedulers/strand.hpp (4)
1-10: LGTM!The license header comment is informative and follows the standard practice.
34-73: LGTM!The
stateclass is well-designed and follows the ASIO programming model. The use ofasio::postandasio::basic_waitable_timeris appropriate for deferring the execution of schedulable functions. The recursive call todefer_foris a good way to handle the case where the schedulable function returns a new duration. Thestateclass is thread-safe because it uses anasio::io_context::strandto serialize the execution of schedulable functions.
75-94: LGTM!The
worker_strategyclass is a simple wrapper around thestateclass and provides the interface for theworkerclass. Theget_disposablemethod returns anone_disposableobject, which is appropriate for this scheduler because it does not have a disposable object. Thenowmethod returns the current time, which is used by theworkerclass to calculate the absolute time for deferred execution.
32-108: LGTM!The
strandclass is a simple scheduler that uses the ASIO library to execute schedulable functions. The use of theworker_strategyclass is a good way to provide the interface for theworkerclass. Thecreate_workermethod returns aworkerobject that can be used to schedule tasks.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- CMakePresets.json (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- CMakePresets.json
CorentinBT
force-pushed
the
asio_scheduler
branch
from
e2dc293 to
b64f664
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (2)
BUILDING.md (1)
31-31: Approve the addition ofRPP_BUILD_ASIO_CODE.The new build configuration option
RPP_BUILD_ASIO_CODEis correctly added and follows the existing pattern for similar options. It is clear and understandable.Consider adding a direct link to the ASIO library installation instructions to aid users in setting up their environment.
src/rpp/rpp/schedulers/current_thread.hpp (1)
87-87: Visibility Change: Public Access Specifier AddedThe addition of the
publicaccess specifier to thecurrent_threadclass enhances its accessibility. This change allows other components, such as thenew_threadclass, to interact more directly withcurrent_thread. While this can be beneficial for certain design patterns, it also increases the risk of misuse or unintended interactions. It is crucial to ensure that this change aligns with the overall design principles of the library and does not expose internal mechanisms that should remain encapsulated.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (18)
- BUILDING.md (1 hunks)
- CMakePresets.json (3 hunks)
- cmake/dependencies.cmake (1 hunks)
- cmake/install-rules.cmake (1 hunks)
- cmake/variables.cmake (2 hunks)
- conanfile.py (2 hunks)
- src/extensions/CMakeLists.txt (1 hunks)
- src/extensions/rppasio/CMakeLists.txt (1 hunks)
- src/extensions/rppasio/rppasio/fwd.hpp (1 hunks)
- src/extensions/rppasio/rppasio/rppasio.hpp (1 hunks)
- src/extensions/rppasio/rppasio/schedulers.hpp (1 hunks)
- src/extensions/rppasio/rppasio/schedulers/fwd.hpp (1 hunks)
- src/extensions/rppasio/rppasio/schedulers/strand.hpp (1 hunks)
- src/rpp/rpp/schedulers/current_thread.hpp (1 hunks)
- src/rpp/rpp/schedulers/details/queue.hpp (2 hunks)
- src/tests/CMakeLists.txt (2 hunks)
- src/tests/rppasio/test_rppasio_scheduler.cpp (1 hunks)
- src/tests/utils/rpp_trompeloil.hpp (2 hunks)
Files skipped from review due to trivial changes (5)
- src/extensions/CMakeLists.txt
- src/extensions/rppasio/CMakeLists.txt
- src/extensions/rppasio/rppasio/fwd.hpp
- src/extensions/rppasio/rppasio/schedulers.hpp
- src/extensions/rppasio/rppasio/schedulers/fwd.hpp
Files skipped from review as they are similar to previous changes (9)
- CMakePresets.json
- cmake/dependencies.cmake
- cmake/install-rules.cmake
- cmake/variables.cmake
- conanfile.py
- src/extensions/rppasio/rppasio/rppasio.hpp
- src/tests/CMakeLists.txt
- src/tests/rppasio/test_rppasio_scheduler.cpp
- src/tests/utils/rpp_trompeloil.hpp
Additional comments not posted (10)
BUILDING.md (1)
29-30: Approve grammatical corrections in option descriptions.The grammatical corrections in the descriptions for
RPP_BUILD_QT_CODEandRPP_BUILD_GRPC_CODEenhance the clarity and correctness of the documentation.src/rpp/rpp/schedulers/current_thread.hpp (6)
Line range hint
89-93: Thread-Local Queue Access: Well-ImplementedThe implementation of the
get_queuemethod using thread-local storage is appropriate and well-suited for the task scheduling mechanism of thecurrent_threadscheduler. This approach ensures that each thread maintains its own queue independently, which is crucial for the correct operation of thread-specific task scheduling.
Line range hint
97-125: Task Execution Logic: Correct and EfficientThe
drain_queuemethod is well-implemented, providing robust logic for executing tasks from the queue. It correctly handles task disposal, scheduling delays, and task rescheduling. This method is central to the functionality of thecurrent_threadscheduler, ensuring that tasks are executed in the correct order and at the right times.
Line range hint
127-147: Improper Value Forwarding in Lambda CapturesThe
defer_formethod uses forwarding for the function, handler, and arguments. While this is generally a good practice to avoid unnecessary copies, it is crucial to ensure that these forwarded values are not captured by reference in any lambdas within this method or passed to other functions that might store them beyond the lifetime of the original objects. This concern aligns with the feedback from the PR comments about potential issues with value forwarding in lambdas. It is recommended to review all lambda captures within this method to ensure they are capturing values correctly and not leading to any unintended side effects.
Line range hint
149-161: Specific Time Point Scheduling: Correctly ImplementedThe
defer_tomethod provides a clear and correct implementation for scheduling tasks at a specific time point. It appropriately uses forwarding for the function, handler, and arguments to optimize performance and reduce unnecessary copies. This method is crucial for precise timing control in task scheduling.
Line range hint
163-165: Current Time Retrieval: Simple and EffectiveThe
nowmethod effectively retrieves the current time point using thedetails::now()function. This utility method is essential for other scheduling methods in thecurrent_threadclass, providing a reliable source for the current time.
Line range hint
167-183: Queue Lifecycle Management: Robust and SafeThe
own_queue_guardmethod provides a robust mechanism for managing the lifecycle of the queue. It uses a boolean flag to ensure that the queue is only cleared when necessary, preventing memory leaks or other issues related to improper resource management. This method is a critical part of ensuring that thecurrent_threadscheduler operates reliably and efficiently.src/rpp/rpp/schedulers/details/queue.hpp (2)
78-78: Approval of the newon_errorfunction.The addition of the
on_errorfunction as a pure virtual method inschedulable_baseis a positive change, enhancing the robustness of error handling by enforcing its implementation in all derived classes.
164-164: Review the implementation ofon_errorinspecific_schedulable.The override of
on_errorinspecific_schedulablecorrectly delegates the error handling to its member'son_errormethod. However, based on the PR comments, it is crucial to ensure that the lambda capture strategy used within this context correctly handles the forwarding of values. Specifically, review whether the values captured bym_argsare being moved appropriately instead of incorrectly usingstd::forward, which could lead to issues as described in the PR comments.Consider verifying the lambda capture and forwarding strategy with a detailed examination of how
m_argsis utilized across different contexts in the codebase.src/extensions/rppasio/rppasio/schedulers/strand.hpp (1)
22-194: Approval: Overall design and structure of thestrandclassThe
strandclass is well-designed, integrating seamlessly with ASIO's io_context to provide a robust mechanism for scheduling tasks in a thread-safe manner. The use of modern C++ features like templates, shared pointers, and lambda expressions enhances its functionality and flexibility.The implementation is generally solid, with good use of C++ best practices and ASIO integration. The concerns raised in previous comments regarding lambda captures are the only major issues needing attention.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/extensions/rppasio/rppasio/schedulers/strand.hpp (1 hunks)
Additional comments not posted (1)
src/extensions/rppasio/rppasio/schedulers/strand.hpp (1)
127-127: Clarification onnow()usage@CorentinBT, regarding your uncertainty about setting
now()here, it seems appropriate to usenow()to update the timepoint of a schedulable after it has been processed. This ensures that the schedulable is re-queued with an updated time based on the current time, which is crucial for accurate scheduling and avoiding drift in time-sensitive applications.Please confirm if this aligns with the intended behavior or if further adjustments are needed.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/extensions/rppasio/rppasio/schedulers/strand.hpp (1 hunks)
Additional comments not posted (2)
src/extensions/rppasio/rppasio/schedulers/strand.hpp (2)
60-73: Review: Lambda captures in asynchronous wait handlerThe lambda expression used in the
defer_with_timehandler captures forwarded arguments. As previously noted, capturing forwarded arguments directly can lead to potential misuse of the moved-from state of these arguments if the lambda is invoked multiple times.To prevent potential issues and ensure the lambda captures the arguments correctly, consider capturing them by moving the arguments instead of forwarding. Here's a suggested change:
- timer->async_wait(asio::bind_executor(m_strand, [self = this->shared_from_this(), timer, fn = std::forward<Fn>(fn), handler = std::forward<Handler>(handler), ... args = std::forward<Args>(args)](const asio::error_code& ec) mutable { + timer->async_wait(asio::bind_executor(m_strand, [self = this->shared_from_this(), timer, fn = std::move(fn), handler = std::move(handler), ... args = std::move(args)](const asio::error_code& ec) mutable {This modification ensures that the lambda captures are appropriate for the intended single invocation per scheduling.
Likely invalid or redundant comment.
44-57: Review: Handling of value forwarding in lambda expressionsThe implementation of
deferusesstd::forwardto forward arguments to a lambda expression. This is generally used to maintain the value category of the arguments passed to a function template. However, when these forwarded arguments are captured by the lambda, they should be captured by value to ensure that they are moved, not forwarded again. This is because forwarding is context-dependent and only makes sense when forwarding to another function that accepts a universal reference.The current implementation captures forwarded arguments in the lambda, which might lead to incorrect behavior or inefficiencies if the lambda is invoked multiple times, as the state of these arguments after the first invocation is indeterminate.
Consider capturing these arguments by value in the lambda to ensure correct and efficient behavior. Here's a suggested change:
- asio::post(asio::bind_executor(m_strand, [self = this->shared_from_this(), fn = std::forward<Fn>(fn), handler = std::forward<Handler>(handler), ... args = std::forward<Args>(args)]() mutable { + asio::post(asio::bind_executor(m_strand, [self = this->shared_from_this(), fn = std::move(fn), handler = std::move(handler), ... args = std::move(args)]() mutable {This change ensures that the lambda captures moved versions of the arguments, which is more appropriate for the use case where the lambda is only supposed to be called once per scheduling.
Likely invalid or redundant comment.
…into asio_scheduler
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/rpp/rpp/schedulers/current_thread.hpp (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/rpp/rpp/schedulers/current_thread.hpp
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/tests/rppasio/test_rppasio_scheduler.cpp (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/tests/rppasio/test_rppasio_scheduler.cpp
|
2 participants
Summary by CodeRabbit
New Features
strandclass for thread-safe task scheduling using ASIO's strand functionality.worker_strategyclass to facilitate task execution strategies within thestrandclass.Documentation
Tests