Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enha: make save_pending_execution cheaper #1911

Merged
merged 2 commits into from
Dec 12, 2024

Conversation

carneiro-cw
Copy link
Contributor

@carneiro-cw carneiro-cw commented Dec 12, 2024

PR Type

Enhancement


Description

  • Implemented PartialEq trait for EvmInput struct in evm_input.rs, enabling efficient comparison with transaction input and block header.
  • Optimized save_pending_execution method in inmemory.rs:
    • Replaced write lock with upgradable read lock for better concurrency.
    • Utilized new PartialEq implementation for simplified conflict checking.
    • Upgraded to write lock only when necessary, reducing lock contention.
  • These changes aim to improve performance and reduce overhead in the transaction execution process.

Changes walkthrough 📝

Relevant files
Enhancement
evm_input.rs
Add PartialEq implementation for EvmInput                               

src/eth/executor/evm_input.rs

  • Implemented PartialEq trait for EvmInput struct, comparing it with a
    tuple of (&TransactionInput, &PendingBlockHeader)
  • The new implementation allows for efficient comparison of EvmInput
    with transaction input and block header
  • +13/-0   
    inmemory.rs
    Optimize save_pending_execution with upgradable read lock

    src/eth/storage/temporary/inmemory.rs

  • Added RwLockUpgradableReadGuard import from parking_lot
  • Modified save_pending_execution method to use upgradable read lock
  • Simplified conflict checking logic using the new PartialEq
    implementation
  • Upgraded read lock to write lock only when necessary
  • +7/-4     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Performance Concern
    The new PartialEq implementation compares multiple fields, which could be expensive for frequent comparisons. Consider if all these comparisons are necessary or if a subset of fields would suffice.

    Error Handling
    The error handling for TransactionEvmInputMismatch is now done before upgrading the lock. Ensure this doesn't introduce any race conditions or inconsistencies.

    Concurrency Management
    The change from write lock to upgradable read lock improves concurrency, but ensure that all necessary modifications are done after upgrading to write lock.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Use a fallible lock upgrade to prevent potential panics and deadlocks

    The RwLockUpgradableReadGuard::upgrade operation might panic if another thread has
    acquired a write lock. Consider using a fallible upgrade method or restructure the
    code to avoid potential deadlocks.

    src/eth/storage/temporary/inmemory.rs [137]

    -let mut pending_block = RwLockUpgradableReadGuard::<InMemoryTemporaryStorageState>::upgrade(pending_block);
    +let mut pending_block = match RwLockUpgradableReadGuard::<InMemoryTemporaryStorageState>::try_upgrade(pending_block) {
    +    Ok(guard) => guard,
    +    Err(_) => return Err(StratusError::LockUpgradeFailure),
    +};
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a critical concurrency issue. Using a fallible upgrade method prevents potential panics and deadlocks, significantly improving the robustness and reliability of the code in multi-threaded scenarios.

    9
    General
    Utilize the implemented PartialEq trait for more consistent and maintainable code

    Consider using the PartialEq trait implementation for EvmInput to simplify the
    comparison in save_pending_execution. This would make the code more consistent and
    easier to maintain.

    src/eth/executor/evm_input.rs [128-133]

    -if tx.evm_input != (&tx.input, &pending_block.block.header) {
    -    let expected_input = EvmInput::from_eth_transaction(&tx.input, &pending_block.block.header);
    +let expected_input = EvmInput::from_eth_transaction(&tx.input, &pending_block.block.header);
    +if tx.evm_input != expected_input {
         return Err(StratusError::TransactionEvmInputMismatch {
             expected: Box::new(expected_input),
             actual: Box::new(tx.evm_input.clone()),
         });
     }
    Suggestion importance[1-10]: 7

    Why: The suggestion improves code consistency and readability by leveraging the newly implemented PartialEq trait. It simplifies the comparison logic, making it more maintainable and less error-prone.

    7

    @carneiro-cw carneiro-cw merged commit 701141a into main Dec 12, 2024
    34 checks passed
    @carneiro-cw carneiro-cw deleted the cheaper_save_pending_execution branch December 12, 2024 13:42
    @gabriel-aranha-cw
    Copy link
    Contributor

    Final benchmark:
    Run ID: bench-4271986061

    Git Info:

    Configuration:

    • Target Account Strategy: Default

    RPS Stats: Max: 1232.00, Min: 535.00, Avg: 1020.91, StdDev: 71.21
    TPS Stats: Max: 1237.00, Min: 874.00, Avg: 991.11, StdDev: 74.34

    Plot: View Plot

    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.

    2 participants