Skip to content

Add benchmarks#687

Merged
AlexInLog merged 11 commits into
v2from
add_benchmarks
Nov 23, 2024
Merged

Add benchmarks#687
AlexInLog merged 11 commits into
v2from
add_benchmarks

Conversation

@AlexInLog

@AlexInLog AlexInLog commented Nov 21, 2024

Copy link
Copy Markdown
Owner

Summary by CodeRabbit

  • New Features

    • Enhanced CI workflow with a new matrix parameter for optimization settings.
    • Added new sections in benchmark tests to evaluate operators with and without disposables.
    • Introduced a new disposables strategy mode for improved management of disposed states.
  • Improvements

    • Updated benchmark result reporting to differentiate between optimized and non-optimized results.
    • Improved clarity in job naming conventions for CI processes.
    • Streamlined workflow for artifact downloads and comment management based on event types.
  • Bug Fixes

    • Adjusted caching mechanisms to align with new configurations.

@coderabbitai

coderabbitai Bot commented Nov 21, 2024

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

The changes in this pull request primarily focus on enhancing the Continuous Integration (CI) workflow and the benchmarking framework by introducing new configuration options for optimization settings. A new matrix parameter, optimization_disabled, is added to the CI jobs, affecting job naming and the handling of build configurations. Additionally, modifications are made to benchmark tests and related scripts to accommodate these new optimization states, including conditional compilation and new test cases. The overall structure of the workflow and benchmarks is preserved while allowing for more granular control over optimizations.

Changes

File Path Change Summary
.github/workflows/ci v2.yml - Added matrix parameter optimization_disabled in tests job.
- Updated job names and Run CMake step to include optimization mode.
- Modified Upload benchmark results step for naming convention.
src/benchmarks/CMakeLists.txt - Added conditional compilation for RPP_DISABLE_DISPOSABLES_OPTIMIZATION in benchmarks target.
src/benchmarks/benchmarks.cpp - Introduced new benchmark sections: "mix operators with disposables and without disposables" and "single disposable and looooooong chain".
src/rpp/rpp/observers/details/fwd.hpp - Added new enumerator Boolean to disposables_mode enum.
- Declared boolean_disposables_strategy class.
ci/process_benchmark_data.py - Enhanced benchmark result handling for optimization states.
- Added variables optimization_off_suffix and optimization_off_title.
.github/workflows/comment_benchmarks_v2.yml - Introduced job step to download workflow run artifacts.
- Refined logic for creating and updating comments based on event types.

Possibly related PRs

  • try to speedup cache #663: The changes in the CI workflow configuration in .github/workflows/ci v2.yml related to caching mechanisms are relevant as they enhance the overall CI process, which is also a focus of the main PR's modifications to the CI workflow.
  • add new benchmarks #676: The addition of a new benchmark section in src/benchmarks/benchmarks.cpp is related to the main PR's focus on enhancing the CI process, particularly in how benchmarks are handled and reported in the CI workflow.

🐇 In the meadow where bunnies play,
New settings bloom, brightening the day.
With optimizations, we leap and bound,
In tests and benchmarks, joy is found!
Hop along, let's celebrate this cheer,
For every change brings us near! 🌼


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 3ee778c and 4a5dc35.

📒 Files selected for processing (1)
  • .github/workflows/comment_benchmarks_v2.yml (1 hunks)
🔇 Additional comments (1)
.github/workflows/comment_benchmarks_v2.yml (1)

Line range hint 30-36: Consider adding error handling and cleanup steps

The artifact download step is correctly positioned and properly configured. However, consider these improvements:

  1. Add error handling in case no artifacts are found
  2. Add a cleanup step to remove artifacts after processing
  3. Consider using a unique subdirectory for artifacts to prevent conflicts
      - name: Download all workflow run artifacts
        uses: dawidd6/action-download-artifact@v6
        with:
          github_token: ${{secrets.GITHUB_TOKEN}}
          path: artifacts
          run_id: ${{github.event.workflow_run.id}}
+         continue-on-error: true
+
+      - name: Verify artifacts
+        run: |
+          if [ ! "$(ls -A artifacts 2>/dev/null)" ]; then
+            echo "No artifacts found to process"
+            exit 1
+          fi

       # ... existing steps ...

+      - name: Cleanup artifacts
+        if: always()
+        run: rm -rf artifacts

Let's verify the latest stable version of the action:

✅ Verification successful

Version v6 is the latest stable release, no version update needed

The workflow is using the latest stable version (v6) of the dawidd6/action-download-artifact action. However, the suggestions for error handling and cleanup remain valid to improve workflow reliability.

  • Add continue-on-error: true to prevent workflow failures when no artifacts exist
  • Add a verification step to check for downloaded artifacts
  • Include a cleanup step using if: always() to ensure artifacts are removed
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check the latest release version of the action
gh api repos/dawidd6/action-download-artifact/releases/latest --jq .tag_name

Length of output: 81


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (11)
src/benchmarks/CMakeLists.txt (2)

23-23: LGTM! Consider adding documentation.

The compile definition is correctly added, but it would be helpful to document its impact on benchmarks in a comment.

+# Control whether disposables optimization is enabled in benchmarks
 target_compile_definitions(${TARGET} PRIVATE "RPP_DISABLE_DISPOSABLES_OPTIMIZATION=${RPP_DISABLE_DISPOSABLES_OPTIMIZATION}")

23-23: Add validation for the RPP_DISABLE_DISPOSABLES_OPTIMIZATION variable.

Consider adding a check to ensure the variable is defined before using it.

+if(NOT DEFINED RPP_DISABLE_DISPOSABLES_OPTIMIZATION)
+    message(FATAL_ERROR "RPP_DISABLE_DISPOSABLES_OPTIMIZATION must be defined")
+endif()
 target_compile_definitions(${TARGET} PRIVATE "RPP_DISABLE_DISPOSABLES_OPTIMIZATION=${RPP_DISABLE_DISPOSABLES_OPTIMIZATION}")
src/rpp/rpp/observers/details/fwd.hpp (2)

28-30: LGTM! Consider enhancing the documentation.

The new Boolean enum value is well-placed and follows the existing pattern. The comment explains its purpose well.

Consider adding an example use case in the documentation to help developers choose between Boolean and other modes:

 // Observer just controls is_disposed or not but upstreams handled via observer_strategy
+// Example: Use this mode when the observer only needs to track disposal state without managing upstream resources
 Boolean = 3

52-55: Enhance documentation to match other strategy classes.

While the documentation provides basic information, consider matching the detail level of other strategy classes in this file.

Consider expanding the documentation:

 /**
- * @brief Just control is_disposed or not via boolean and ignore upstreams at all
+ * @brief Strategy that manages disposal state via a boolean flag without upstream management
+ * 
+ * This strategy provides a lightweight alternative when only disposal state tracking is needed,
+ * without the overhead of managing upstream disposables. It's useful in scenarios where the
+ * observer strategy handles upstream resources independently.
  */
src/tests/utils/disposable_observable.hpp (1)

119-120: LGTM! Comprehensive disposal verification added.

These assertions effectively verify disposal states after completion, maintaining symmetry with the error case checks. Together, they ensure proper resource cleanup in both completion and error scenarios.

Consider extracting these common assertions into a helper function to avoid duplication and make future maintenance easier:

inline void verify_disposal_states(const auto& obs, const auto& d) {
    CHECK(obs.is_disposed());
    CHECK(d.is_disposed());
}
.github/workflows/ci v2.yml (2)

212-212: Consider adding optimization mode to artifact path.

While the artifact name includes the optimization state, consider also including it in the results file path to prevent any potential overwrites if artifacts are extracted to the same directory.

-        path: ${{github.workspace}}/build/test_results/benchmarks_results.json
+        path: ${{github.workspace}}/build/test_results/benchmarks_results${{ matrix.optimization_disabled.postfix}}.json

Line range hint 143-212: Consider enhancing benchmark result analysis.

The current implementation successfully captures benchmark results for both optimized and non-optimized builds. Consider these enhancements:

  1. Add a job to compare benchmark results between optimized and non-optimized builds
  2. Implement performance regression detection by comparing results with the base branch
  3. Add performance regression thresholds to fail the workflow when significant regressions are detected

This would help catch performance regressions early in the development cycle.

src/rpp/rpp/sources/concat.hpp (1)

Line range hint 84-106: Document thread safety guarantees and memory ordering.

The drain implementation uses atomic operations and handles concurrency, but would benefit from explicit documentation about:

  1. Thread safety guarantees
  2. Memory ordering requirements for atomic operations
  3. Synchronization between disposal and drain operations

Consider adding documentation like:

/**
 * Drains the observables sequentially while handling concurrent disposal.
 * Thread safety: Safe for concurrent disposal attempts.
 * Memory ordering: Uses seq_cst for is_inside_drain to ensure visibility
 * across threads.
 */
src/benchmarks/benchmarks.cpp (3)

816-816: Reconsider delay duration for meaningful benchmarks

Using a delay of 0 seconds might not effectively test the timing behavior. Consider using a small but non-zero delay to better measure the performance impact.

-    | rpp::ops::delay(std::chrono::seconds{0}, rpp::schedulers::immediate{})
+    | rpp::ops::delay(std::chrono::milliseconds{1}, rpp::schedulers::immediate{})

804-823: Add RxCPP comparison for completeness

Other benchmark sections include comparisons with RxCPP implementation. Consider adding the RxCPP equivalent to maintain consistency and provide comparative benchmarks.

Add the RxCPP implementation:

TEST_RXCPP([&]() {
    rxcpp::subjects::subject<int> s{};
    s.get_observable()
        | rxcpp::operators::filter([](int v) -> bool { return v; })
        | rxcpp::operators::finally([]() { ankerl::nanobench::doNotOptimizeAway(1); })
        | rxcpp::operators::flat_map([](int v) { 
            return rxcpp::observable<>::from(v * 2, v * 3); 
          })
        | rxcpp::operators::delay(std::chrono::milliseconds{1}, 
            rxcpp::identity_immediate())
        | rxcpp::operators::subscribe<int>(
            [](int v) { ankerl::nanobench::doNotOptimizeAway(v); });
    s.get_subscriber().on_next(1);
    s.get_subscriber().on_completed();
});

812-813: Consider simplifying the map and concat operations

The current implementation creates intermediate observables and concatenates them. This could be simplified using flat_map for better performance.

-    | rpp::ops::map([](int v) { return rpp::source::just(v * 2, v * 3); })
-    | rpp::ops::concat()
+    | rpp::ops::flat_map([](int v) { return rpp::source::just(v * 2, v * 3); })
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 96c6e33 and 3445ff0.

📒 Files selected for processing (11)
  • .github/workflows/ci v2.yml (3 hunks)
  • src/benchmarks/CMakeLists.txt (1 hunks)
  • src/benchmarks/benchmarks.cpp (1 hunks)
  • src/rpp/rpp/observers/details/disposables_strategy.hpp (1 hunks)
  • src/rpp/rpp/observers/details/fwd.hpp (3 hunks)
  • src/rpp/rpp/operators/concat.hpp (2 hunks)
  • src/rpp/rpp/operators/delay.hpp (1 hunks)
  • src/rpp/rpp/operators/merge.hpp (1 hunks)
  • src/rpp/rpp/operators/retry.hpp (2 hunks)
  • src/rpp/rpp/sources/concat.hpp (2 hunks)
  • src/tests/utils/disposable_observable.hpp (2 hunks)
🧰 Additional context used
📓 Learnings (2)
src/rpp/rpp/observers/details/disposables_strategy.hpp (1)
Learnt from: victimsnino
PR: victimsnino/ReactivePlusPlus#667
File: src/rpp/rpp/observers/details/disposable_strategy.hpp:37-37
Timestamp: 2024-11-10T04:25:44.618Z
Learning: In `local_disposable_strategy`, `m_is_disposed` is safely used as a plain `bool` because the class is intended for single-threaded use or is protected by synchronization mechanisms.
src/rpp/rpp/operators/concat.hpp (1)
Learnt from: victimsnino
PR: victimsnino/ReactivePlusPlus#673
File: src/rpp/rpp/operators/concat.hpp:59-59
Timestamp: 2024-11-10T04:25:44.618Z
Learning: In the `concat_disposable` class in `src/rpp/rpp/operators/concat.hpp`, within the `drain` method, it's acceptable to call `get_observer()->on_completed()` even when `is_disposed()` returns true, because `refcounted.dispose()` could cause disposing of `this` object.
🔇 Additional comments (13)
src/rpp/rpp/observers/details/fwd.hpp (1)

75-90: LGTM! Verify integration with optimization settings.

The strategy deduction logic is correctly updated to handle the new Boolean mode, both with and without optimizations enabled.

Let's verify that the new mode is properly handled across the codebase:

✅ Verification successful

Boolean mode integration is properly handled across the codebase

The verification shows that:

  • The new Boolean mode is correctly used in multiple operators (concat, delay, merge, retry) with proper preferred_disposables_mode declarations
  • The boolean_disposables_strategy class is properly declared and referenced
  • All conditional logic paths properly handle the new mode, including the strategy deduction logic
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of Boolean mode and boolean_disposables_strategy

# Check for any direct usage of Boolean mode
echo "Checking for Boolean mode usage:"
rg "disposables_mode::Boolean" -A 2

# Check for boolean_disposables_strategy instantiations
echo "Checking for boolean_disposables_strategy usage:"
rg "boolean_disposables_strategy" -A 2

# Check for potential switch/if statements that need to handle the new mode
echo "Checking for potential switch/if statements on disposables_mode:"
rg "switch.*disposables_mode|if.*disposables_mode" -A 5

Length of output: 5939

src/rpp/rpp/operators/retry.hpp (2)

80-80: LGTM: Simplified disposal check improves code clarity.

The removal of the locally_disposed check simplifies the disposal logic by relying solely on the state's disposal status. This change reduces state management complexity while maintaining the required functionality.


44-44: Verify impact of disposables mode change on retry behavior.

The change from None to Boolean disposables mode aligns with the broader refactoring of disposables management across observer strategies. However, we should verify that retry's error handling and resubscription behavior remains correct under the new mode.

✅ Verification successful

Change to Boolean disposables mode is safe and verified

The change from None to Boolean disposables mode in retry operator is verified to be correct based on:

  1. Comprehensive test coverage in test_retry.cpp specifically verifying disposal behavior:

    • Test case "retry disposes on looping" confirms proper disposal of upstream
    • Test case "retry satisfies disposable contracts" validates disposable behavior
  2. Implementation shows simplified disposal logic that only relies on state's disposal status, which aligns with the Boolean mode

  3. Test cases verify retry's core functionality remains intact under various scenarios including:

    • Multiple retry attempts
    • Threading scenarios
    • Error handling
    • Stack overflow protection
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for test cases that verify retry behavior with disposables
rg -l "TEST.*retry.*dispos" 

# Look for similar disposables mode changes in related operators
ast-grep --pattern 'preferred_disposables_mode = rpp::details::observers::disposables_mode::Boolean'

Length of output: 195


Script:

#!/bin/bash
# Check retry test cases for disposal behavior
cat src/tests/rpp/test_retry.cpp src/tests/rpp/test_retry_when.cpp

# Look for other operators' disposables mode settings
ast-grep --pattern 'preferred_disposables_mode = rpp::details::observers::disposables_mode::$_'

# Check for any disposal-related changes in retry implementation
rg -A 5 "is_disposed\(\)" src/rpp/rpp/operators/retry.hpp

Length of output: 19976

src/tests/utils/disposable_observable.hpp (1)

108-109: LGTM! Important disposal state verification added.

These assertions effectively verify that both the observer and its disposable are properly disposed of after an error occurs, ensuring proper resource cleanup.

.github/workflows/ci v2.yml (2)

143-143: LGTM! Good addition of optimization matrix.

The addition of the optimization matrix parameter is well-structured and provides clear visibility into the optimization state through the job naming.

Also applies to: 147-147


202-202: Verify CMake variable naming convention.

The CMake configuration properly integrates the optimization flag, but let's verify if the variable name follows the project's convention.

✅ Verification successful

The variable RPP_DISABLE_DISPOSABLES_OPTIMIZATION follows the project's CMake variable naming convention

The verification shows that the project consistently uses the RPP_ prefix followed by uppercase letters and underscores for CMake variables (e.g., RPP_BUILD_TESTS, RPP_DEVELOPER_MODE, RPP_BUILD_RXCPP). The variable RPP_DISABLE_DISPOSABLES_OPTIMIZATION follows this exact same pattern.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check CMake variable naming patterns in the project
rg -g 'CMakeLists.txt' 'RPP_[A-Z_]+' --no-filename | sort -u

Length of output: 1205

src/rpp/rpp/operators/concat.hpp (2)

148-148: LGTM! Simplified disposal state management.

The updated implementation correctly centralizes disposal state in the inner child disposable, which aligns with the Boolean disposables mode and maintains correctness based on previous learnings about disposal behavior.


118-118: LGTM! Verify consistency with other observer strategies.

The change from None to Boolean aligns with the broader initiative to standardize disposables management across the library.

Let's verify that this change is consistent with other similar strategies:

✅ Verification successful

Change to Boolean disposables mode is consistent with similar operators

The change from None to Boolean disposables mode aligns with other complex operators that manage subscription state:

  • delay operator uses Boolean mode
  • merge operator uses Boolean mode
  • retry operator uses Boolean mode

This standardization improves consistency in how disposal state is handled across the library's operators.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other observer strategies and their disposables mode
ast-grep --pattern 'struct $name_observer_strategy {
  $$$
  static constexpr auto preferred_disposables_mode = $mode;
  $$$
}'

Length of output: 129


Script:

#!/bin/bash
# Let's try a different approach to search for disposables_mode declarations
rg -A 2 "preferred_disposables_mode\s*=" --type cpp

Length of output: 16696

src/rpp/rpp/sources/concat.hpp (2)

53-53: Verify the impact of disposables mode change on cleanup behavior.

The change from None to Boolean disposables mode aligns with similar updates across other observer strategies, but we should verify its impact on cleanup timing and performance.

✅ Verification successful

Let's gather more information about the test cases and other disposables mode usage to make a proper assessment.


Based on the gathered information, I can now provide the final response:

Change to Boolean disposables mode is consistent with similar operators

The change from None to Boolean disposables mode in concat source operator aligns with other operators that manage multiple subscriptions, such as merge and retry. The test cases in test_concat.cpp verify proper cleanup behavior, particularly in the "concat tracks actual upstream" and "concat disposes on looping" test cases. This change improves resource management consistency.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other strategies to verify consistent disposables mode usage
ast-grep --pattern 'struct $_ {
  $$$
  static constexpr auto preferred_disposables_mode = $_;
  $$$
}'

# Search for tests validating cleanup behavior
rg -l "TEST.*concat.*cleanup|TEST.*concat.*dispose" --type cpp

Length of output: 200


Script:

#!/bin/bash
# Check the test file content for cleanup/dispose tests
cat src/tests/rpp/test_concat.cpp

# Look for other disposables_mode usage patterns
rg "disposables_mode::" -A 2 -B 2

# Check for any related changes in the same PR
rg "preferred_disposables_mode" --type cpp

Length of output: 48414


70-70: Review thread safety of disposal state management.

The simplified is_disposed() implementation removes local state tracking, which is good for maintainability. However, we should verify:

  1. Thread safety in concurrent scenarios
  2. Proper synchronization with the drain operation
✅ Verification successful

Based on the search results, I can now generate the final response. The code shows proper thread-safety mechanisms in place:

Thread safety of disposal state is properly maintained

The implementation is thread-safe because:

  1. The concat_state_t uses std::atomic<bool> is_inside_drain for synchronization
  2. The base disposable implementation uses atomic operations for disposal state (m_disposed.load(std::memory_order::seq_cst))
  3. The composite disposable also uses atomic state management (m_current_state.load(std::memory_order::seq_cst))

The simplified is_disposed() implementation correctly delegates to the underlying thread-safe state management.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for concurrent access patterns
rg -A 5 "is_disposed.*atomic|atomic.*is_disposed" --type cpp

# Look for related test cases
rg -l "TEST.*concat.*concurrent|TEST.*concat.*thread" --type cpp

Length of output: 129


Script:

#!/bin/bash
# Look for the state class implementation to check atomic operations
ast-grep --pattern 'class state {
  $$$
  is_disposed() {
    $$$
  }
  $$$
}'

# Search for disposal-related operations in concat source
rg -B 2 -A 5 "disposed|dispose" src/rpp/rpp/sources/concat.hpp

# Look for concurrent tests specifically for concat
fd "concat.*test" -e cpp -e hpp | xargs rg "thread|concurrent|async"

Length of output: 65354


Script:

#!/bin/bash
# Look for state implementation and atomic operations
rg -B 2 -A 5 "class concat_state|struct concat_state" src/rpp/rpp/sources/concat.hpp

# Check for synchronization mechanisms in state class
rg -B 2 -A 5 "mutex|atomic|lock" src/rpp/rpp/sources/concat.hpp

# Look for disposal implementation in base classes
rg -B 2 -A 5 "is_disposed\(\)|void dispose\(\)" src/rpp/rpp/disposables/

Length of output: 8131

src/rpp/rpp/operators/delay.hpp (2)

72-72: Request: Add benchmark results for disposables mode change.

Since this PR focuses on benchmarks, please provide performance comparison results between None and Boolean disposables modes for the delay operator, particularly focusing on:

  • Memory usage
  • Disposal time
  • Impact on emission scheduling

This will help validate the optimization and provide baseline metrics for future improvements.


72-72: LGTM! Verify consistency with other operators.

The change from None to Boolean disposables mode aligns with the optimization pattern seen in other operator strategies.

Let's verify the consistency across other operator strategies:

src/rpp/rpp/operators/merge.hpp (1)

51-51: LGTM! Verify benchmark impact of disposables optimization.

The change from None to Boolean disposables mode aligns with the optimization efforts. Since this affects a performance-critical merge operation path, please ensure:

  1. The benchmarks show improvement or at least no regression
  2. Similar changes in related files (concat.hpp, delay.hpp, retry.hpp) maintain consistent behavior

Let's verify the consistency of this change across related files:

✅ Verification successful

Optimization pattern confirmed: Boolean mode applied to concurrent operators

The verification shows a clear pattern where Boolean disposables mode is consistently applied to operators that handle concurrent operations (merge, concat, delay, retry). Other operators maintain None mode, suggesting this is an intentional optimization strategy for concurrent scenarios.

  • merge.hpp: Boolean mode ✓
  • concat.hpp: Boolean mode ✓ (except for one implementation, which might need review)
  • delay.hpp: Boolean mode ✓
  • retry.hpp: Boolean mode ✓
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if all related files have been updated consistently
# Expected: All files should use Boolean mode if changed

# Search for preferred_disposables_mode in related files
echo "Checking disposables mode in related files..."
rg "preferred_disposables_mode\s*=\s*rpp::details::observers::disposables_mode::(None|Boolean)" src/rpp/rpp/operators/

Length of output: 5750

Comment thread src/rpp/rpp/observers/details/disposables_strategy.hpp
Comment thread src/benchmarks/benchmarks.cpp
@github-actions

github-actions Bot commented Nov 21, 2024

Copy link
Copy Markdown
Contributor

BENCHMARK RESULTS (AUTOGENERATED)

ci-ubuntu-gcc

General

name rxcpp rpp prev rpp ratio rpp no optimization
Subscribe empty callbacks to empty observable 301.99 ns 1.54 ns 1.87 ns 0.83 1.90 ns
Subscribe empty callbacks to empty observable via pipe operator 301.93 ns 1.54 ns 1.85 ns 0.83 1.85 ns

Sources

name rxcpp rpp prev rpp ratio rpp no optimization
from array of 1 - create + subscribe + immediate 689.36 ns 0.31 ns 0.31 ns 1.00 0.31 ns
from array of 1 - create + subscribe + current_thread 1045.21 ns 3.42 ns 3.42 ns 1.00 3.71 ns
concat_as_source of just(1 immediate) create + subscribe 2245.55 ns 112.80 ns 114.25 ns 0.99 120.92 ns
defer from array of 1 - defer + create + subscribe + immediate 731.61 ns 0.31 ns 0.31 ns 1.00 0.31 ns
interval - interval + take(3) + subscribe + immediate 2164.71 ns 59.19 ns 59.23 ns 1.00 59.23 ns
interval - interval + take(3) + subscribe + current_thread 2954.53 ns 32.43 ns 32.42 ns 1.00 33.99 ns
from array of 1 - create + as_blocking + subscribe + new_thread 30234.03 ns 27921.19 ns 28162.83 ns 0.99 28676.50 ns
from array of 1000 - create + as_blocking + subscribe + new_thread 42265.37 ns 49899.48 ns 51101.16 ns 0.98 50694.48 ns
concat_as_source of just(1 immediate) and just(1,2 immediate)create + subscribe 3539.47 ns 133.93 ns 141.13 ns 0.95 151.19 ns

Filtering Operators

name rxcpp rpp prev rpp ratio rpp no optimization
immediate_just+take(1)+subscribe 1120.92 ns 0.31 ns 0.31 ns 1.00 0.31 ns
immediate_just+filter(true)+subscribe 842.37 ns 0.31 ns 0.31 ns 1.00 0.31 ns
immediate_just(1,2)+skip(1)+subscribe 1003.61 ns 0.31 ns 0.31 ns 1.00 0.31 ns
immediate_just(1,1,2)+distinct_until_changed()+subscribe 916.01 ns 0.31 ns 0.31 ns 1.00 0.31 ns
immediate_just(1,2)+first()+subscribe 1282.72 ns 0.31 ns 0.31 ns 1.00 0.31 ns
immediate_just(1,2)+last()+subscribe 912.67 ns 0.31 ns 0.31 ns 1.00 0.31 ns
immediate_just+take_last(1)+subscribe 1106.99 ns 18.20 ns 18.84 ns 0.97 19.44 ns
immediate_just(1,2,3)+element_at(1)+subscribe 870.32 ns 0.31 ns 0.31 ns 1.00 0.31 ns

Schedulers

name rxcpp rpp prev rpp ratio rpp no optimization
immediate scheduler create worker + schedule 293.54 ns 0.62 ns 1.54 ns 0.40 1.54 ns
current_thread scheduler create worker + schedule 361.86 ns 4.94 ns 4.63 ns 1.07 4.32 ns
current_thread scheduler create worker + schedule + recursive schedule 840.38 ns 60.78 ns 60.52 ns 1.00 60.62 ns

Transforming Operators

name rxcpp rpp prev rpp ratio rpp no optimization
immediate_just+map(v*2)+subscribe 838.13 ns 0.31 ns 0.31 ns 1.00 0.31 ns
immediate_just+scan(10, std::plus)+subscribe 886.09 ns 0.31 ns 0.31 ns 1.00 0.31 ns
immediate_just+flat_map(immediate_just(v*2))+subscribe 2323.68 ns 142.31 ns 139.59 ns 1.02 167.66 ns
immediate_just+buffer(2)+subscribe 1545.93 ns 13.89 ns 14.19 ns 0.98 18.23 ns
immediate_just+window(2)+subscribe + subscsribe inner 2378.49 ns 1328.42 ns 1323.82 ns 1.00 1296.19 ns

Conditional Operators

name rxcpp rpp prev rpp ratio rpp no optimization
immediate_just+take_while(false)+subscribe 842.60 ns - - 0.00 -
immediate_just+take_while(true)+subscribe 856.07 ns 0.31 ns 0.31 ns 1.00 0.31 ns

Utility Operators

name rxcpp rpp prev rpp ratio rpp no optimization
immediate_just(1)+subscribe_on(immediate)+subscribe 1977.94 ns 0.31 ns 0.31 ns 1.00 0.31 ns

Combining Operators

name rxcpp rpp prev rpp ratio rpp no optimization
immediate_just(immediate_just(1), immediate_just(1)) + merge() + subscribe 3438.28 ns 141.42 ns 168.04 ns 0.84 174.35 ns
immediate_just(1) + merge_with(immediate_just(2)) + subscribe 3734.69 ns 154.19 ns 157.58 ns 0.98 162.76 ns
immediate_just(1) + with_latest_from(immediate_just(2)) + subscribe - 128.07 ns 138.35 ns 0.93 156.08 ns
immediate_just(immediate_just(1),immediate_just(1)) + switch_on_next() + subscribe 3541.19 ns 405.66 ns 400.74 ns 1.01 390.80 ns
immediate_just(1) + zip(immediate_just(2)) + subscribe 2144.01 ns 212.21 ns 214.30 ns 0.99 220.79 ns
immediate_just(immediate_just(1), immediate_just(1)) + concat() + subscribe 3105.93 ns 219.21 ns 223.54 ns 0.98 257.97 ns

Subjects

name rxcpp rpp prev rpp ratio rpp no optimization
publish_subject with 1 observer - on_next 34.46 ns 14.67 ns 14.67 ns 1.00 14.96 ns
subscribe 100 observers to publish_subject 200706.83 ns 15662.69 ns 15790.18 ns 0.99 16435.48 ns
100 on_next to 100 observers to publish_subject 27141.05 ns 17261.10 ns 17286.17 ns 1.00 20368.91 ns

Scenarios

name rxcpp rpp prev rpp ratio rpp no optimization
basic sample 1444.38 ns 13.27 ns 12.97 ns 1.02 24.38 ns
basic sample with immediate scheduler 1405.92 ns 5.55 ns 5.55 ns 1.00 19.14 ns
mix operators with disposables and without disposables 6323.21 ns 1447.75 ns - 0.00 1862.33 ns
single disposable and looooooong indentity chain 23636.95 ns 1017.81 ns - 0.00 5158.86 ns

Aggregating Operators

name rxcpp rpp prev rpp ratio rpp no optimization
immediate_just+reduce(10, std::plus)+subscribe 912.74 ns 0.31 ns 0.31 ns 1.00 0.31 ns

Error Handling Operators

name rxcpp rpp prev rpp ratio rpp no optimization
create(on_next(1), on_error())+on_error_resume_next(immediate_just(2)))+subscribe 2081.74 ns 1009.72 ns 986.74 ns 1.02 1000.19 ns
create(on_error())+retry(1)+subscribe 608.88 ns 113.05 ns 121.60 ns 0.93 116.79 ns

ci-macos

General

name rxcpp rpp prev rpp ratio rpp no optimization
Subscribe empty callbacks to empty observable 1476.55 ns 0.98 ns 0.47 ns 2.08 0.70 ns
Subscribe empty callbacks to empty observable via pipe operator 1461.29 ns 1.06 ns 0.47 ns 2.24 0.70 ns

Sources

name rxcpp rpp prev rpp ratio rpp no optimization
from array of 1 - create + subscribe + immediate 3000.44 ns 0.33 ns 0.24 ns 1.38 0.23 ns
from array of 1 - create + subscribe + current_thread 2715.81 ns 41.88 ns 34.27 ns 1.22 31.24 ns
concat_as_source of just(1 immediate) create + subscribe 6343.87 ns 359.01 ns 340.37 ns 1.05 316.95 ns
defer from array of 1 - defer + create + subscribe + immediate 2308.20 ns 0.27 ns 0.24 ns 1.13 0.23 ns
interval - interval + take(3) + subscribe + immediate 6180.24 ns 139.33 ns 117.05 ns 1.19 114.56 ns
interval - interval + take(3) + subscribe + current_thread 7575.32 ns 118.92 ns 99.17 ns 1.20 97.63 ns
from array of 1 - create + as_blocking + subscribe + new_thread 91657.85 ns 91593.75 ns 89869.77 ns 1.02 79108.29 ns
from array of 1000 - create + as_blocking + subscribe + new_thread 91743.92 ns 93883.08 ns 89302.27 ns 1.05 83790.29 ns
concat_as_source of just(1 immediate) and just(1,2 immediate)create + subscribe 10487.69 ns 449.60 ns 368.19 ns 1.22 360.23 ns

Filtering Operators

name rxcpp rpp prev rpp ratio rpp no optimization
immediate_just+take(1)+subscribe 2855.80 ns 0.23 ns 0.24 ns 0.98 0.23 ns
immediate_just+filter(true)+subscribe 2102.25 ns 0.23 ns 0.24 ns 0.98 0.23 ns
immediate_just(1,2)+skip(1)+subscribe 2770.18 ns 0.23 ns 0.23 ns 1.00 0.23 ns
immediate_just(1,1,2)+distinct_until_changed()+subscribe 2132.30 ns 0.47 ns 0.47 ns 1.00 0.47 ns
immediate_just(1,2)+first()+subscribe 3191.52 ns 0.23 ns 0.23 ns 1.00 0.23 ns
immediate_just(1,2)+last()+subscribe 2383.95 ns 0.23 ns 0.23 ns 1.00 0.23 ns
immediate_just+take_last(1)+subscribe 3006.95 ns 0.23 ns 0.23 ns 1.00 0.23 ns
immediate_just(1,2,3)+element_at(1)+subscribe 2562.60 ns 0.25 ns 0.24 ns 1.05 0.23 ns

Schedulers

name rxcpp rpp prev rpp ratio rpp no optimization
immediate scheduler create worker + schedule 1054.88 ns 1.18 ns 0.93 ns 1.26 0.70 ns
current_thread scheduler create worker + schedule 1476.46 ns 40.87 ns 34.05 ns 1.20 30.88 ns
current_thread scheduler create worker + schedule + recursive schedule 2349.17 ns 245.54 ns 198.05 ns 1.24 191.38 ns

Transforming Operators

name rxcpp rpp prev rpp ratio rpp no optimization
immediate_just+map(v*2)+subscribe 2095.12 ns 4.22 ns 4.20 ns 1.00 4.12 ns
immediate_just+scan(10, std::plus)+subscribe 2322.18 ns 0.47 ns 0.47 ns 1.00 0.46 ns
immediate_just+flat_map(immediate_just(v*2))+subscribe 5272.48 ns 373.22 ns 371.74 ns 1.00 375.17 ns
immediate_just+buffer(2)+subscribe 2479.06 ns 64.86 ns 64.55 ns 1.00 66.94 ns
immediate_just+window(2)+subscribe + subscsribe inner 5315.13 ns 2365.67 ns 2355.76 ns 1.00 2412.76 ns

Conditional Operators

name rxcpp rpp prev rpp ratio rpp no optimization
immediate_just+take_while(false)+subscribe 2070.23 ns - - 0.00 -
immediate_just+take_while(true)+subscribe 2094.19 ns 0.23 ns 0.23 ns 0.99 0.24 ns

Utility Operators

name rxcpp rpp prev rpp ratio rpp no optimization
immediate_just(1)+subscribe_on(immediate)+subscribe 5067.86 ns 5.50 ns 5.22 ns 1.05 4.90 ns

Combining Operators

name rxcpp rpp prev rpp ratio rpp no optimization
immediate_just(immediate_just(1), immediate_just(1)) + merge() + subscribe 8250.03 ns 512.75 ns 415.22 ns 1.23 396.76 ns
immediate_just(1) + merge_with(immediate_just(2)) + subscribe 9269.56 ns 496.96 ns 408.51 ns 1.22 390.52 ns
immediate_just(1) + with_latest_from(immediate_just(2)) + subscribe - 592.05 ns 448.25 ns 1.32 427.32 ns
immediate_just(immediate_just(1),immediate_just(1)) + switch_on_next() + subscribe 7979.44 ns 950.78 ns 923.63 ns 1.03 882.93 ns
immediate_just(1) + zip(immediate_just(2)) + subscribe 5112.00 ns 821.86 ns 829.68 ns 0.99 782.53 ns
immediate_just(immediate_just(1), immediate_just(1)) + concat() + subscribe 8752.56 ns 761.05 ns 646.56 ns 1.18 614.98 ns

Subjects

name rxcpp rpp prev rpp ratio rpp no optimization
publish_subject with 1 observer - on_next 78.44 ns 53.05 ns 281.89 ns 0.19 50.35 ns
subscribe 100 observers to publish_subject 360802.00 ns 45089.89 ns 64190.60 ns 0.70 40307.88 ns
100 on_next to 100 observers to publish_subject 53965.23 ns 20890.81 ns 26821.00 ns 0.78 20909.24 ns

Scenarios

name rxcpp rpp prev rpp ratio rpp no optimization
basic sample 2783.99 ns 68.41 ns 71.74 ns 0.95 84.30 ns
basic sample with immediate scheduler 2945.35 ns 19.75 ns 18.70 ns 1.06 32.15 ns
mix operators with disposables and without disposables 14248.52 ns 3502.22 ns - 0.00 4303.65 ns
single disposable and looooooong indentity chain 25802.47 ns 1729.76 ns - 0.00 6372.61 ns

Aggregating Operators

name rxcpp rpp prev rpp ratio rpp no optimization
immediate_just+reduce(10, std::plus)+subscribe 2605.27 ns 0.28 ns 0.24 ns 1.16 0.23 ns

Error Handling Operators

name rxcpp rpp prev rpp ratio rpp no optimization
create(on_next(1), on_error())+on_error_resume_next(immediate_just(2)))+subscribe 7975.54 ns 4817.52 ns 4166.91 ns 1.16 4037.71 ns
create(on_error())+retry(1)+subscribe 2038.13 ns 316.67 ns 286.28 ns 1.11 279.17 ns

ci-ubuntu-clang

General

name rxcpp rpp prev rpp ratio rpp no optimization
Subscribe empty callbacks to empty observable 278.31 ns 1.55 ns 1.54 ns 1.00 1.55 ns
Subscribe empty callbacks to empty observable via pipe operator 272.42 ns 1.54 ns 1.55 ns 1.00 1.54 ns

Sources

name rxcpp rpp prev rpp ratio rpp no optimization
from array of 1 - create + subscribe + immediate 579.64 ns 0.31 ns 0.31 ns 1.00 0.31 ns
from array of 1 - create + subscribe + current_thread 786.21 ns 4.01 ns 4.01 ns 1.00 4.02 ns
concat_as_source of just(1 immediate) create + subscribe 2353.61 ns 129.51 ns 130.49 ns 0.99 128.77 ns
defer from array of 1 - defer + create + subscribe + immediate 775.25 ns 0.31 ns 0.31 ns 1.00 0.31 ns
interval - interval + take(3) + subscribe + immediate 2186.37 ns 58.26 ns 58.35 ns 1.00 58.26 ns
interval - interval + take(3) + subscribe + current_thread 3169.70 ns 30.86 ns 30.88 ns 1.00 31.50 ns
from array of 1 - create + as_blocking + subscribe + new_thread 26724.90 ns 27791.92 ns 27554.74 ns 1.01 28885.44 ns
from array of 1000 - create + as_blocking + subscribe + new_thread 36108.54 ns 35364.85 ns 32730.79 ns 1.08 39134.20 ns
concat_as_source of just(1 immediate) and just(1,2 immediate)create + subscribe 3635.41 ns 148.78 ns 148.58 ns 1.00 148.07 ns

Filtering Operators

name rxcpp rpp prev rpp ratio rpp no optimization
immediate_just+take(1)+subscribe 1173.64 ns 0.31 ns 0.31 ns 1.00 0.31 ns
immediate_just+filter(true)+subscribe 851.06 ns 0.31 ns 0.31 ns 1.00 0.31 ns
immediate_just(1,2)+skip(1)+subscribe 1062.78 ns 0.31 ns 0.31 ns 1.00 0.31 ns
immediate_just(1,1,2)+distinct_until_changed()+subscribe 862.94 ns 0.62 ns 0.31 ns 2.00 0.31 ns
immediate_just(1,2)+first()+subscribe 1384.77 ns 0.31 ns 0.31 ns 1.00 0.31 ns
immediate_just(1,2)+last()+subscribe 989.77 ns 0.31 ns 0.31 ns 1.00 0.31 ns
immediate_just+take_last(1)+subscribe 1180.34 ns 0.31 ns 0.31 ns 1.00 0.31 ns
immediate_just(1,2,3)+element_at(1)+subscribe 859.25 ns 0.31 ns 0.31 ns 1.00 0.31 ns

Schedulers

name rxcpp rpp prev rpp ratio rpp no optimization
immediate scheduler create worker + schedule 276.38 ns 0.63 ns 0.63 ns 1.00 0.63 ns
current_thread scheduler create worker + schedule 393.12 ns 4.02 ns 4.03 ns 1.00 4.33 ns
current_thread scheduler create worker + schedule + recursive schedule 846.49 ns 54.32 ns 62.61 ns 0.87 55.03 ns

Transforming Operators

name rxcpp rpp prev rpp ratio rpp no optimization
immediate_just+map(v*2)+subscribe 843.36 ns 0.31 ns 0.31 ns 1.00 0.31 ns
immediate_just+scan(10, std::plus)+subscribe 961.62 ns 0.31 ns 0.62 ns 0.50 0.31 ns
immediate_just+flat_map(immediate_just(v*2))+subscribe 2233.94 ns 140.16 ns 135.85 ns 1.03 137.36 ns
immediate_just+buffer(2)+subscribe 1515.63 ns 13.58 ns 13.89 ns 0.98 14.20 ns
immediate_just+window(2)+subscribe + subscsribe inner 2438.82 ns 917.20 ns 918.83 ns 1.00 955.86 ns

Conditional Operators

name rxcpp rpp prev rpp ratio rpp no optimization
immediate_just+take_while(false)+subscribe 836.43 ns - - 0.00 -
immediate_just+take_while(true)+subscribe 836.27 ns 0.31 ns 0.31 ns 1.00 0.31 ns

Utility Operators

name rxcpp rpp prev rpp ratio rpp no optimization
immediate_just(1)+subscribe_on(immediate)+subscribe 1999.47 ns 0.31 ns 0.31 ns 1.00 0.31 ns

Combining Operators

name rxcpp rpp prev rpp ratio rpp no optimization
immediate_just(immediate_just(1), immediate_just(1)) + merge() + subscribe 3329.54 ns 158.90 ns 159.89 ns 0.99 154.45 ns
immediate_just(1) + merge_with(immediate_just(2)) + subscribe 3743.25 ns 138.00 ns 139.10 ns 0.99 139.28 ns
immediate_just(1) + with_latest_from(immediate_just(2)) + subscribe - 142.69 ns 141.78 ns 1.01 137.44 ns
immediate_just(immediate_just(1),immediate_just(1)) + switch_on_next() + subscribe 3427.93 ns 376.52 ns 378.00 ns 1.00 377.76 ns
immediate_just(1) + zip(immediate_just(2)) + subscribe 2198.27 ns 198.13 ns 207.68 ns 0.95 197.05 ns
immediate_just(immediate_just(1), immediate_just(1)) + concat() + subscribe 3227.74 ns 222.57 ns 220.19 ns 1.01 223.27 ns

Subjects

name rxcpp rpp prev rpp ratio rpp no optimization
publish_subject with 1 observer - on_next 53.97 ns 17.47 ns 17.25 ns 1.01 18.25 ns
subscribe 100 observers to publish_subject 211753.60 ns 16134.45 ns 16091.58 ns 1.00 17021.08 ns
100 on_next to 100 observers to publish_subject 42752.04 ns 23528.86 ns 23467.86 ns 1.00 23593.31 ns

Scenarios

name rxcpp rpp prev rpp ratio rpp no optimization
basic sample 1301.01 ns 11.12 ns 10.81 ns 1.03 20.38 ns
basic sample with immediate scheduler 1308.57 ns 5.88 ns 6.17 ns 0.95 6.79 ns
mix operators with disposables and without disposables 6450.49 ns 1182.87 ns - 0.00 1521.86 ns
single disposable and looooooong indentity chain 27539.00 ns 1246.50 ns - 0.00 4665.40 ns

Aggregating Operators

name rxcpp rpp prev rpp ratio rpp no optimization
immediate_just+reduce(10, std::plus)+subscribe 988.57 ns 0.31 ns 0.31 ns 1.00 0.31 ns

Error Handling Operators

name rxcpp rpp prev rpp ratio rpp no optimization
create(on_next(1), on_error())+on_error_resume_next(immediate_just(2)))+subscribe 2191.22 ns 1161.54 ns 1156.23 ns 1.00 1154.73 ns
create(on_error())+retry(1)+subscribe 652.89 ns 138.47 ns 138.51 ns 1.00 138.23 ns

ci-windows

General

name rxcpp rpp prev rpp ratio rpp no optimization
Subscribe empty callbacks to empty observable 564.22 ns 2.16 ns 2.16 ns 1.00 1.85 ns
Subscribe empty callbacks to empty observable via pipe operator 584.93 ns 2.16 ns 2.16 ns 1.00 1.85 ns

Sources

name rxcpp rpp prev rpp ratio rpp no optimization
from array of 1 - create + subscribe + immediate 1160.93 ns 5.86 ns 5.24 ns 1.12 4.93 ns
from array of 1 - create + subscribe + current_thread 1417.24 ns 15.74 ns 15.46 ns 1.02 15.45 ns
concat_as_source of just(1 immediate) create + subscribe 3710.87 ns 166.98 ns 174.80 ns 0.96 178.09 ns
defer from array of 1 - defer + create + subscribe + immediate 1194.92 ns 5.55 ns 5.24 ns 1.06 5.24 ns
interval - interval + take(3) + subscribe + immediate 3684.26 ns 139.79 ns 139.83 ns 1.00 142.11 ns
interval - interval + take(3) + subscribe + current_thread 3417.46 ns 59.87 ns 60.21 ns 0.99 62.46 ns
from array of 1 - create + as_blocking + subscribe + new_thread 117866.67 ns 111190.00 ns 115290.00 ns 0.96 110477.78 ns
from array of 1000 - create + as_blocking + subscribe + new_thread 126400.00 ns 129075.00 ns 135300.00 ns 0.95 129987.50 ns
concat_as_source of just(1 immediate) and just(1,2 immediate)create + subscribe 5382.16 ns 201.11 ns 202.00 ns 1.00 209.63 ns

Filtering Operators

name rxcpp rpp prev rpp ratio rpp no optimization
immediate_just+take(1)+subscribe 1820.03 ns 19.42 ns 19.44 ns 1.00 21.35 ns
immediate_just+filter(true)+subscribe 1620.80 ns 18.50 ns 18.50 ns 1.00 21.59 ns
immediate_just(1,2)+skip(1)+subscribe 2007.44 ns 17.89 ns 17.89 ns 1.00 21.61 ns
immediate_just(1,1,2)+distinct_until_changed()+subscribe 1351.87 ns 20.67 ns 20.69 ns 1.00 27.24 ns
immediate_just(1,2)+first()+subscribe 2364.20 ns 18.21 ns 18.20 ns 1.00 19.44 ns
immediate_just(1,2)+last()+subscribe 1494.85 ns 19.14 ns 19.13 ns 1.00 22.83 ns
immediate_just+take_last(1)+subscribe 2001.12 ns 64.10 ns 69.76 ns 0.92 67.68 ns
immediate_just(1,2,3)+element_at(1)+subscribe 1637.98 ns 20.98 ns 20.98 ns 1.00 21.63 ns

Schedulers

name rxcpp rpp prev rpp ratio rpp no optimization
immediate scheduler create worker + schedule 483.44 ns 4.94 ns 4.32 ns 1.14 4.32 ns
current_thread scheduler create worker + schedule 657.97 ns 11.85 ns 11.68 ns 1.01 11.13 ns
current_thread scheduler create worker + schedule + recursive schedule 1078.22 ns 98.63 ns 95.64 ns 1.03 100.95 ns

Transforming Operators

name rxcpp rpp prev rpp ratio rpp no optimization
immediate_just+map(v*2)+subscribe 1319.89 ns 18.82 ns 18.82 ns 1.00 21.58 ns
immediate_just+scan(10, std::plus)+subscribe 1457.80 ns 20.98 ns 20.97 ns 1.00 23.76 ns
immediate_just+flat_map(immediate_just(v*2))+subscribe 3844.19 ns 183.94 ns 189.43 ns 0.97 204.60 ns
immediate_just+buffer(2)+subscribe 2368.12 ns 64.41 ns 63.52 ns 1.01 69.93 ns
immediate_just+window(2)+subscribe + subscsribe inner 4065.48 ns 1274.66 ns 1321.18 ns 0.96 1312.55 ns

Conditional Operators

name rxcpp rpp prev rpp ratio rpp no optimization
immediate_just+take_while(false)+subscribe 1322.72 ns 17.58 ns 17.58 ns 1.00 19.11 ns
immediate_just+take_while(true)+subscribe 1336.32 ns 18.51 ns 18.50 ns 1.00 21.58 ns

Utility Operators

name rxcpp rpp prev rpp ratio rpp no optimization
immediate_just(1)+subscribe_on(immediate)+subscribe 3602.06 ns 11.10 ns 11.11 ns 1.00 11.11 ns

Combining Operators

name rxcpp rpp prev rpp ratio rpp no optimization
immediate_just(immediate_just(1), immediate_just(1)) + merge() + subscribe 5125.81 ns 204.43 ns 204.81 ns 1.00 227.45 ns
immediate_just(1) + merge_with(immediate_just(2)) + subscribe 5725.13 ns 197.15 ns 187.40 ns 1.05 196.83 ns
immediate_just(1) + with_latest_from(immediate_just(2)) + subscribe - 189.50 ns 194.18 ns 0.98 197.85 ns
immediate_just(immediate_just(1),immediate_just(1)) + switch_on_next() + subscribe 6139.57 ns 437.11 ns 456.56 ns 0.96 454.68 ns
immediate_just(1) + zip(immediate_just(2)) + subscribe 3741.45 ns 522.65 ns 524.38 ns 1.00 525.19 ns
immediate_just(immediate_just(1), immediate_just(1)) + concat() + subscribe 4865.28 ns 322.81 ns 326.95 ns 0.99 364.97 ns

Subjects

name rxcpp rpp prev rpp ratio rpp no optimization
publish_subject with 1 observer - on_next 36.26 ns 19.76 ns 20.76 ns 0.95 20.81 ns
subscribe 100 observers to publish_subject 278325.00 ns 30214.71 ns 27674.42 ns 1.09 28786.49 ns
100 on_next to 100 observers to publish_subject 55080.00 ns 33367.74 ns 36103.57 ns 0.92 39219.23 ns

Scenarios

name rxcpp rpp prev rpp ratio rpp no optimization
basic sample 2044.27 ns 96.16 ns 96.02 ns 1.00 112.31 ns
basic sample with immediate scheduler 2030.04 ns 66.92 ns 68.06 ns 0.98 81.77 ns
mix operators with disposables and without disposables 10508.25 ns 1918.01 ns - 0.00 2613.36 ns
single disposable and looooooong indentity chain 27069.77 ns 1740.82 ns - 0.00 6324.42 ns

Aggregating Operators

name rxcpp rpp prev rpp ratio rpp no optimization
immediate_just+reduce(10, std::plus)+subscribe 1456.90 ns 19.11 ns 19.11 ns 1.00 22.82 ns

Error Handling Operators

name rxcpp rpp prev rpp ratio rpp no optimization
create(on_next(1), on_error())+on_error_resume_next(immediate_just(2)))+subscribe 2314.03 ns 349.85 ns 361.03 ns 0.97 365.53 ns
create(on_error())+retry(1)+subscribe 1833.02 ns 139.77 ns 138.06 ns 1.01 138.71 ns

@codecov

codecov Bot commented Nov 21, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.61%. Comparing base (d309d58) to head (4a5dc35).
Report is 3 commits behind head on v2.

Additional details and impacted files
@@           Coverage Diff           @@
##               v2     #687   +/-   ##
=======================================
  Coverage   98.61%   98.61%           
=======================================
  Files         156      156           
  Lines        9786     9786           
=======================================
  Hits         9650     9650           
  Misses        136      136           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
.github/workflows/ci v2.yml (2)

202-202: Consider improving CMake flag readability.

While the CMake configuration is correct, consider breaking the long configurePresetAdditionalArgs into multiple lines for better readability.

-        configurePresetAdditionalArgs: "['-DCMAKE_BUILD_TYPE=${{ matrix.build_type.config }}', '-DCMAKE_CONFIGURATION_TYPES=${{ matrix.build_type.config }}', '-DRPP_DISABLE_DISPOSABLES_OPTIMIZATION=${{matrix.optimization_disabled.mode}}']"
+        configurePresetAdditionalArgs: >-
+          [
+            '-DCMAKE_BUILD_TYPE=${{ matrix.build_type.config }}',
+            '-DCMAKE_CONFIGURATION_TYPES=${{ matrix.build_type.config }}',
+            '-DRPP_DISABLE_DISPOSABLES_OPTIMIZATION=${{matrix.optimization_disabled.mode}}'
+          ]

143-143: Well-structured implementation of benchmark optimization testing.

The changes effectively implement a comprehensive benchmarking system with optimization control:

  1. Matrix configuration provides clear control over optimization states
  2. Job naming and artifacts maintain consistency throughout the workflow
  3. CMake integration ensures proper propagation of optimization settings

Consider documenting the purpose and impact of these optimization settings in the repository's CI documentation.

Also applies to: 147-147, 202-202, 212-212

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 764a74e and 8e29a2b.

📒 Files selected for processing (1)
  • .github/workflows/ci v2.yml (3 hunks)
🔇 Additional comments (2)
.github/workflows/ci v2.yml (2)

212-212: LGTM! Consistent artifact naming.

The artifact naming convention aligns well with the job naming, making it easy to identify optimization states in the benchmark results.


143-143: LGTM! Matrix configuration for optimization control.

The addition of the optimization_disabled matrix parameter effectively enables testing both optimized and non-optimized scenarios. The job naming clearly reflects the optimization state.

Let's verify the matrix combinations are working as expected:

Also applies to: 147-147

✅ Verification successful

Matrix configuration for optimization control is correctly implemented

The verification confirms that:

  • The optimization_disabled matrix parameter is properly defined with two modes (0 and 1)
  • The parameter is correctly used in:
    • Job naming via ${{ matrix.optimization_disabled.postfix}}
    • CMake configuration via -DRPP_DISABLE_DISPOSABLES_OPTIMIZATION=${{matrix.optimization_disabled.mode}}
    • Artifact naming via the postfix parameter

The matrix combinations will generate separate jobs for both optimized and non-optimized scenarios for each configuration, build type, and test type, providing comprehensive test coverage.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that the workflow generates the expected job combinations

# Check if the workflow file contains all necessary matrix combinations
rg -A 5 'matrix:.*optimization_disabled:' .github/workflows/ci\ v2.yml

Length of output: 73


Script:

#!/bin/bash
# Let's try a different approach to verify the matrix configuration
# First, let's check the entire matrix configuration block
rg -B 5 -A 10 'matrix:' '.github/workflows/ci v2.yml'

# Also check for the optimization_disabled parameter usage
rg 'optimization_disabled' '.github/workflows/ci v2.yml'

Length of output: 2073

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (4)
ci/process_benchmark_data.py (2)

32-38: LGTM! Consider extracting the source constant for better maintainability.

The logic for handling optimization-disabled results is clear and correct. However, the hardcoded 'rpp' string in the list comprehension could be extracted as a constant alongside the other configuration values.

Consider this minor improvement:

optimization_off_suffix = " (Optimizations disabled)"
optimization_off_title = f"rpp{optimization_off_suffix}"
+RPP_SOURCE = "rpp"
 
if is_optimization:
    platform = platform[:-len(optimization_off_suffix)]
-    results = [{**r, "source": optimization_off_title} for r in results if r["source"] == 'rpp']
+    results = [{**r, "source": optimization_off_title} for r in results if r["source"] == RPP_SOURCE]

Line range hint 54-72: Consider adding validation and improving table formatting.

While the changes correctly add support for non-optimized results, there are a few areas that could be improved:

  1. Add validation for the table width consistency:
 headers = ["name", "rxcpp", "rpp", "prev rpp", "ratio", "rpp no optimization"]
 separators = ["---"] * len(headers)
-print("name | rxcpp | rpp | prev rpp | ratio | rpp no optimization")
-print("--- | --- | --- | --- | --- | ---")
+print(" | ".join(headers))
+print(" | ".join(separators))
  1. Consider adding a helper function for value retrieval to reduce code duplication:
def get_metric_value(values, source):
    return values.get(source, {}).get('median(elapsed)')

# Usage:
rpp_value = get_metric_value(cur_vals, 'rpp')
rxcpp_value = get_metric_value(cur_vals, 'rxcpp')
no_optimization_value = get_metric_value(cur_vals, optimization_off_title)
🧰 Tools
🪛 Ruff (0.7.0)

74-74: f-string without any placeholders

Remove extraneous f prefix

(F541)

src/benchmarks/benchmarks.cpp (2)

804-823: LGTM! Consider consolidating redundant filter operations.

The benchmark section effectively tests the performance of mixing operators with and without disposables. However, there are three identical filter operations that could be consolidated for better performance.

Consider consolidating the three filter operations into a single one:

 s.get_observable()
-    | rpp::ops::filter([](int v) -> bool { return v; })
     | rpp::ops::finally([]() noexcept { ankerl::nanobench::doNotOptimizeAway(1); })
     | rpp::ops::map([](int v) { return rpp::source::just(v * 2, v * 3); })
     | rpp::ops::concat()
-    | rpp::ops::filter([](int v) -> bool { return v; })
     | rpp::ops::delay(std::chrono::seconds{0}, rpp::schedulers::immediate{})
-    | rpp::ops::filter([](int v) -> bool { return v; })
+    | rpp::ops::filter([](int v) -> bool { return v; })
     | rpp::ops::subscribe([](int v) { ankerl::nanobench::doNotOptimizeAway(v); });

816-816: Consider removing unnecessary delay operator.

The delay operator with 0 seconds using immediate scheduler doesn't add any meaningful delay or scheduling behavior to the benchmark. Consider removing it unless there's a specific reason for including it.

     | rpp::ops::concat()
-    | rpp::ops::delay(std::chrono::seconds{0}, rpp::schedulers::immediate{})
     | rpp::ops::filter([](int v) -> bool { return v; })
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 8e29a2b and 3da117e.

📒 Files selected for processing (5)
  • .github/workflows/ci v2.yml (3 hunks)
  • ci/process_benchmark_data.py (3 hunks)
  • src/benchmarks/CMakeLists.txt (1 hunks)
  • src/benchmarks/benchmarks.cpp (1 hunks)
  • src/rpp/rpp/observers/details/fwd.hpp (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/benchmarks/CMakeLists.txt
  • src/rpp/rpp/observers/details/fwd.hpp
🔇 Additional comments (4)
.github/workflows/ci v2.yml (4)

143-143: LGTM! Clean matrix configuration for optimization control.

The new matrix parameter is well-structured and follows the existing pattern, with clear mode values and descriptive postfixes.


147-147: LGTM! Enhanced job visibility with optimization state.

The job naming clearly indicates when optimizations are disabled, making it easier to track and differentiate between optimization states in the CI dashboard.


202-202: Verify the CMake variable usage in the project.

The CMake configuration correctly passes the optimization control flag. Let's verify its usage in the project's CMake files.

✅ Verification successful

CMake variable is properly used and integrated in the codebase

The variable RPP_DISABLE_DISPOSABLES_OPTIMIZATION is correctly:

  • Defined as a compile definition in src/benchmarks/CMakeLists.txt
  • Used in the codebase for conditional compilation in src/rpp/rpp/observers/details/fwd.hpp to control disposables optimization strategy
  • Properly propagated from CI workflow to the build system
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the CMake variable usage and its impact

# Check for the CMake variable usage in CMake files
rg -l "RPP_DISABLE_DISPOSABLES_OPTIMIZATION" --type cmake

# Check for any conditional compilation based on this flag
rg "#ifdef\s+RPP_DISABLE_DISPOSABLES_OPTIMIZATION" --type cpp

Length of output: 151


Script:

#!/bin/bash
# Let's check the actual usage context in the CMakeLists.txt file
cat src/benchmarks/CMakeLists.txt

# Also search for any related preprocessor definitions or compiler flags
rg "RPP_DISABLE_DISPOSABLES_OPTIMIZATION" -A 3 -B 3

# Check for any other potential optimization-related flags
rg "DISABLE.*OPTIMIZATION" --type cmake

Length of output: 2819


212-212: Verify benchmark processing script compatibility.

The artifact naming correctly includes the optimization state. Ensure that any downstream scripts processing these benchmark results handle the new naming convention correctly.

✅ Verification successful

Benchmark processing script is compatible with the new naming convention

The script process_benchmark_data.py explicitly handles the optimization disabled postfix through:

  • Detecting optimization state via optimization_off_suffix = " (Optimizations disabled)"
  • Correctly processing platform names with this suffix
  • Including results in a dedicated "rpp no optimization" column in the benchmark table
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check benchmark processing script compatibility

# Look for benchmark processing scripts
rg -l "benchmarks_results\.json" --type python

# Check for postfix handling in these scripts
rg "optimization.*disabled" --type python

Length of output: 154


Script:

#!/bin/bash
# Search for any files containing 'benchmark' and 'results.json'
rg -l "benchmark.*results\.json"

# Search for files that might process artifacts
fd -e py -e js -e ts -e sh . | xargs rg -l "artifact|benchmark.*results"

# Look for workflow files that might consume these artifacts
fd "workflow" .github/ | xargs cat

Length of output: 272


Script:

#!/bin/bash
# Check the content of the benchmark processing script
cat ci/process_benchmark_data.py

# Check the workflow files correctly
fd -e yml -e yaml . .github/workflows/ | xargs cat

Length of output: 12013

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 3da117e and 23da9bc.

📒 Files selected for processing (1)
  • src/benchmarks/benchmarks.cpp (1 hunks)
🔇 Additional comments (1)
src/benchmarks/benchmarks.cpp (1)

804-843: LGTM with existing review comment!

The implementation is correct. Note that there's an existing review comment about consolidating the redundant filter operations, which is still applicable.

Comment thread src/benchmarks/benchmarks.cpp Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
src/benchmarks/benchmarks.cpp (1)

844-929: Consider documenting the benchmark's purpose

This benchmark appears to be testing the performance overhead of chaining multiple map operations. Consider adding a comment explaining the purpose and expected performance characteristics being measured.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between bc42c66 and 3ee778c.

📒 Files selected for processing (1)
  • src/benchmarks/benchmarks.cpp (1 hunks)
🔇 Additional comments (2)
src/benchmarks/benchmarks.cpp (2)

809-819: Consolidate redundant filter operations

The implementation contains three identical filter operations that can be consolidated.


853-885: Refactor the repeated map operations chain

The implementation contains 32 identical map operations that can be refactored using a helper function.

@sonarqubecloud

Copy link
Copy Markdown

@AlexInLog AlexInLog marked this pull request as ready for review November 23, 2024 10:33
@AlexInLog AlexInLog merged commit db32585 into v2 Nov 23, 2024
@AlexInLog AlexInLog deleted the add_benchmarks branch November 23, 2024 10:34
@coderabbitai coderabbitai Bot mentioned this pull request Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant