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 faster library for storage cache #1895

Merged

Conversation

marcospb19-cw
Copy link
Contributor

@marcospb19-cw marcospb19-cw commented Dec 3, 2024

PR Type

Enhancement


Description

  • Replaced LruCache with quick_cache::sync::Cache for improved performance in storage caching
  • Removed Mutex locks, simplifying the code and potentially reducing contention
  • Eliminated SmallVec usage, streamlining batch operations
  • Updated dependencies: removed lru, added quick_cache
  • Simplified cache operations (insert, get, clear) for better readability and maintenance
  • Potential performance improvements due to the use of a more efficient caching library

Changes walkthrough 📝

Relevant files
Enhancement
cache.rs
Optimize storage cache implementation                                       

src/eth/storage/cache.rs

  • Replaced LruCache with quick_cache::sync::Cache
  • Removed Mutex locks and parking_lot dependency
  • Simplified cache operations (insert, get, clear)
  • Removed SmallVec for batch operations
  • +14/-33 
    Dependencies
    Cargo.toml
    Update dependencies for new cache library                               

    Cargo.toml

  • Removed lru dependency
  • Added quick_cache dependency version 0.6.9
  • +1/-1     

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

    Copy link

    github-actions bot commented Dec 3, 2024

    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 implementation uses fixed-size caches (100,000 for slots and 20,000 for accounts). Validate if these sizes are appropriate for the expected workload and if they might lead to frequent cache evictions.

    Error Handling
    The new implementation doesn't use expect for potential errors, which might make debugging more difficult if issues arise. Consider adding appropriate error handling or explanatory comments.

    Tracing
    The cache operations (insert, get, clear) are not instrumented with tracing events. Consider adding tracing to monitor cache performance and usage patterns.

    Copy link

    github-actions bot commented Dec 3, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Implement error handling for cache operations to improve robustness

    Consider adding error handling or logging for potential cache insertion failures, as
    the quick_cache library's insert method may fail silently if the cache is full.

    src/eth/storage/cache.rs [33-35]

     pub fn cache_slot(&self, address: Address, slot: Slot) {
    -    self.slot_cache.insert((address, slot.index), slot.value);
    +    if !self.slot_cache.insert((address, slot.index), slot.value) {
    +        // Log or handle the case where insertion fails
    +        log::warn!("Failed to insert slot into cache: cache full");
    +    }
     }
    Suggestion importance[1-10]: 5

    Why: The suggestion to add error handling for cache insertions is valid and could improve the robustness of the code. However, it's a minor improvement as the default behavior of quick_cache is likely sufficient for most use cases.

    5

    @marcospb19-cw
    Copy link
    Contributor Author

    /benchmark --target-account-strategy=partial0.33

    @gabriel-aranha-cw
    Copy link
    Contributor

    Benchmark:
    Run ID: bench-684589687

    Git Info:

    Configuration:

    • Target Account Strategy: Override (Partial(0.33))

    RPS Stats: Max: 1342.00, Min: 643.00, Avg: 1170.64, StdDev: 67.76
    TPS Stats: Max: 1333.00, Min: 1012.00, Avg: 1135.74, StdDev: 73.22

    Plot: View Plot

    @marcospb19-cw
    Copy link
    Contributor Author

    /benchmark --target-account-strategy=partial0.5

    @gabriel-aranha-cw
    Copy link
    Contributor

    Benchmark:
    Run ID: bench-2722894004

    Git Info:

    Configuration:

    • Target Account Strategy: Override (Partial(0.5))

    RPS Stats: Max: 1378.00, Min: 973.00, Avg: 1247.69, StdDev: 57.76
    TPS Stats: Max: 1396.00, Min: 1014.00, Avg: 1210.19, StdDev: 69.68

    Plot: View Plot

    @marcospb19-cw
    Copy link
    Contributor Author

    /benchmark --target-account-strategy=partial0.5

    @gabriel-aranha-cw
    Copy link
    Contributor

    Benchmark:
    Run ID: bench-4116690297

    Git Info:

    Configuration:

    • Target Account Strategy: Override (Partial(0.5))

    RPS Stats: Max: 1358.00, Min: 619.00, Avg: 1222.47, StdDev: 66.12
    TPS Stats: Max: 1359.00, Min: 1021.00, Avg: 1186.43, StdDev: 66.50

    Plot: View Plot

    @marcospb19-cw marcospb19-cw force-pushed the enha-use-faster-cache-library-for-storage-cache branch from 7021e5d to 5859440 Compare December 4, 2024 17:39
    @marcospb19-cw marcospb19-cw force-pushed the enha-use-faster-cache-library-for-storage-cache branch from 5859440 to cd08acb Compare December 4, 2024 17:39
    @marcospb19-cw marcospb19-cw enabled auto-merge (squash) December 4, 2024 17:39
    @marcospb19-cw marcospb19-cw merged commit 7e09405 into main Dec 4, 2024
    34 checks passed
    @marcospb19-cw marcospb19-cw deleted the enha-use-faster-cache-library-for-storage-cache branch December 4, 2024 17:46
    @gabriel-aranha-cw
    Copy link
    Contributor

    Final benchmark:
    Run ID: bench-7709445

    Git Info:

    Configuration:

    • Target Account Strategy: Default (Random)

    RPS Stats: Max: 1288.00, Min: 554.00, Avg: 1120.67, StdDev: 76.11
    TPS Stats: Max: 1267.00, Min: 943.00, Avg: 1087.99, StdDev: 77.32

    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