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: use parking_lot mutex instead of std::sync #1902

Merged
merged 6 commits into from
Dec 10, 2024
Merged

Conversation

carneiro-cw
Copy link
Contributor

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

User description

Parking lot mutexes are faster AND RwLock is fairer. However they don't have a poison detection mechanism. Maybe we should keep the std::sync mutexes in places like the executor or the miner if poisoning is actually concern.

PR Type

Enhancement


Description

  • Replaced std::sync::Mutex with parking_lot::Mutex across multiple files for improved performance
  • Removed custom MutexExt trait and its usage, simplifying the codebase
  • Updated mutex locking syntax to use the new parking_lot::Mutex methods
  • Kept MutexResultExt trait, although its usage was removed in the shown changes
  • This change affects core components like the executor, miner, storage, and global state management

Changes walkthrough 📝

Relevant files
Enhancement
executor.rs
Replace std::sync::Mutex with parking_lot::Mutex                 

src/eth/executor/executor.rs

  • Replaced std::sync::Mutex with parking_lot::Mutex
  • Removed usage of MutexExt trait
  • Updated mutex locking syntax
  • +2/-3     
    miner.rs
    Switch to parking_lot::Mutex in miner module                         

    src/eth/miner/miner.rs

  • Replaced std::sync::Mutex with parking_lot::Mutex
  • Removed usage of MutexExt and MutexResultExt traits
  • Updated mutex locking syntax throughout the file
  • +10/-17 
    rocks_state.rs
    Implement parking_lot::Mutex in rocks_state                           

    src/eth/storage/permanent/rocks/rocks_state.rs

  • Replaced std::sync::Mutex with parking_lot::Mutex
  • Removed usage of MutexExt trait
  • Updated mutex locking syntax
  • +2/-4     
    ext.rs
    Remove MutexExt trait and related code                                     

    src/ext.rs

  • Removed MutexExt trait and its implementation
  • Kept MutexResultExt trait (unused in the shown changes)
  • +0/-16   
    globals.rs
    Integrate parking_lot::Mutex in globals module                     

    src/globals.rs

  • Replaced std::sync::Mutex with parking_lot::Mutex
  • Removed usage of MutexExt trait
  • Updated mutex locking syntax
  • +3/-4     

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


    PR Type

    Enhancement


    Description

    • Replaced std::sync::Mutex and std::sync::RwLock with parking_lot::Mutex and parking_lot::RwLock across multiple files for improved performance.
    • Removed custom MutexExt trait and its usage, simplifying the codebase.
    • Updated mutex and rwlock locking syntax to use the new parking_lot methods.
    • Removed error handling for poisoned locks, as parking_lot locks don't have a poison mechanism.
    • Kept MutexResultExt trait, although its usage was removed in the shown changes.
    • This change affects core components like the executor, miner, storage, and global state management.

    Changes walkthrough 📝

    Relevant files
    Enhancement
    executor.rs
    Replace std::sync::Mutex with parking_lot::Mutex                 

    src/eth/executor/executor.rs

  • Replaced std::sync::Mutex with parking_lot::Mutex
  • Removed usage of MutexExt trait
  • Updated mutex locking syntax
  • +2/-3     
    miner.rs
    Replace std::sync locks with parking_lot equivalents         

    src/eth/miner/miner.rs

  • Replaced std::sync::Mutex and std::sync::RwLock with parking_lot
    equivalents
  • Removed usage of MutexExt and MutexResultExt traits
  • Simplified locking operations by removing error handling for poisoned
    locks
  • +13/-28 
    rpc_context.rs
    Replace std::sync::RwLock with parking_lot::RwLock             

    src/eth/rpc/rpc_context.rs

  • Replaced std::sync::RwLock with parking_lot::RwLock
  • Simplified consensus read and write operations by removing error
    handling for poisoned locks
  • +4/-14   
    inmemory.rs
    Replace std::sync::RwLock with parking_lot::RwLock             

    src/eth/storage/permanent/inmemory.rs

  • Replaced std::sync::RwLock with parking_lot::RwLock
  • Updated lock methods to use parking_lot syntax
  • +5/-5     
    rocks_state.rs
    Replace std::sync::Mutex with parking_lot::Mutex                 

    src/eth/storage/permanent/rocks/rocks_state.rs

  • Replaced std::sync::Mutex with parking_lot::Mutex for metrics feature
  • Updated locking syntax to use parking_lot methods
  • +2/-4     
    inmemory.rs
    Replace std::sync::RwLock with parking_lot::RwLock             

    src/eth/storage/temporary/inmemory.rs

  • Replaced std::sync::RwLock with parking_lot::RwLock
  • Updated locking operations to use parking_lot syntax
  • Removed unwrap calls from lock operations
  • +12/-20 
    ext.rs
    Remove MutexExt trait                                                                       

    src/ext.rs

  • Removed MutexExt trait and its implementation
  • Kept MutexResultExt trait
  • +0/-16   
    globals.rs
    Replace std::sync::Mutex with parking_lot::Mutex                 

    src/globals.rs

  • Replaced std::sync::Mutex with parking_lot::Mutex
  • Updated locking operations to use parking_lot syntax
  • Removed usage of MutexExt trait
  • +3/-4     

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

    @carneiro-cw carneiro-cw marked this pull request as draft December 5, 2024 20:03
    Copy link

    github-actions bot commented Dec 5, 2024

    PR Reviewer Guide 🔍

    (Review updated until commit b63a909)

    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

    Tracing Improvement
    The use of tokio::spawn or tokio::spawn_blocking should be replaced with spawn_named or spawn_blocking_named functions for better tracing and monitoring.

    Error Logging
    When logging errors, ensure the original error is logged in a field called 'reason' for consistency and better error tracking.

    Tracing Event Formatting
    Ensure tracing events do not use formatted messages with dynamic fields. Instead, suggest adding these dynamic fields as tracing fields.

    Copy link

    github-actions bot commented Dec 5, 2024

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @carneiro-cw
    Copy link
    Contributor Author

    /benchmark

    @gabriel-aranha-cw
    Copy link
    Contributor

    Benchmark:
    Run ID: bench-755068278

    Git Info:

    Configuration:

    • Target Account Strategy: Default

    RPS Stats: Max: 1265.00, Min: 685.00, Avg: 1110.02, StdDev: 71.68
    TPS Stats: Max: 1284.00, Min: 923.00, Avg: 1076.38, StdDev: 80.29

    Plot: View Plot

    @carneiro-cw
    Copy link
    Contributor Author

    /benchmark

    @carneiro-cw carneiro-cw marked this pull request as ready for review December 9, 2024 18:08
    Copy link

    github-actions bot commented Dec 9, 2024

    Persistent review updated to latest commit b63a909

    Copy link

    github-actions bot commented Dec 9, 2024

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @carneiro-cw carneiro-cw enabled auto-merge (squash) December 10, 2024 21:17
    @carneiro-cw carneiro-cw merged commit 426da2f into main Dec 10, 2024
    34 checks passed
    @carneiro-cw carneiro-cw deleted the parking_lot_mutex branch December 10, 2024 21:36
    @gabriel-aranha-cw
    Copy link
    Contributor

    Final benchmark:
    Run ID: bench-3697973122

    Git Info:

    Configuration:

    • Target Account Strategy: Default

    RPS Stats: Max: 1278.00, Min: 772.00, Avg: 1057.73, StdDev: 78.24
    TPS Stats: Max: 1264.00, Min: 875.00, Avg: 1025.30, StdDev: 84.25

    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