Skip to content

Conversation

@jimmygchen
Copy link
Member

Issue Addressed

Addresses performance issues with AttesterCache::maybe_cache_state identified through tracing, where the method appeared slow due to lock contention.

Proposed Changes

Implements double-checked locking pattern in maybe_cache_state to reduce write lock contention:

  1. Fast path (cache hit): Check with read lock first. If key exists, return immediately without taking write lock.
  2. Slow path (cache miss): Create AttesterCacheValue outside the write lock to avoid blocking other operations during expensive committee cache initialization.
  3. Write lock with double-check: Only take write lock for insertion, with a second contains_key check to handle race conditions where another thread may have inserted while we were computing.

Files changed:

  • beacon_node/beacon_chain/src/attester_cache.rs

Additional Info

Performance Benefits

  • Eliminates write lock contention for cache hits (the common case in production)
  • Removes expensive computation from critical section: Committee cache initialization (CommitteeLengths::new()state.initialize_committee_cache()) no longer blocks all other cache operations
  • Maintains correctness: Properly handles race conditions with double-checked locking pattern

Pattern Consistency

This optimization follows the same pattern already used in load_and_cache_state (lines 317-326), which explicitly documents this approach in its comments to avoid duplicate state reads.

Testing

Verified with cargo check -p beacon_chain - compiles successfully with no issues.

  Implements double-checked locking pattern to avoid write lock contention:
  - Fast path: Check with read lock first (cache hit case)
  - Slow path: Compute AttesterCacheValue outside write lock
  - Only take write lock for insertion, with race condition handling

  This prevents expensive committee cache initialization from blocking
  all other cache operations, reducing contention in high-concurrency
  scenarios where multiple validators are attesting simultaneously.
@michaelsproul
Copy link
Member

I think another option is just to delete the attester cache, I'm not convinced it's necessary when we have tree-states and the committee cache

@jimmygchen
Copy link
Member Author

Superceded by #8469

@jimmygchen jimmygchen closed this Nov 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants