Add reader/writer locks to cache stripe for reduced contention #12601
+395
−5
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,183 @@ | ||
| # Cache Reader/Writer Lock Optimization Analysis | ||
|
|
||
| ## Current Problem: Exclusive Locks for Read Operations | ||
|
|
||
| ### Current Implementation | ||
| The cache currently uses **exclusive try locks** (`CACHE_TRY_LOCK`) for all operations: | ||
| - Cache lookups (read-only) | ||
| - Directory probes (read-only) | ||
| - Cache writes (modifying) | ||
| - Directory inserts/removes (modifying) | ||
|
|
||
| **Result**: Even read-only operations acquire exclusive locks, preventing concurrent reads. | ||
|
|
||
| ## Why Reader/Writer Locks Would Help | ||
|
|
||
| ### Available Infrastructure | ||
| Apache Traffic Server already has **two** reader/writer lock implementations: | ||
|
|
||
| 1. **`ts::shared_mutex`** (`include/tsutil/TsSharedMutex.h`) | ||
| - Wrapper around `pthread_rwlock_t` | ||
| - Provides writer-starvation prevention | ||
| - Standard interface: `lock()`, `lock_shared()`, `try_lock()`, `try_lock_shared()` | ||
|
|
||
| 2. **`ts::bravo::shared_mutex`** (`include/tsutil/Bravo.h`) | ||
| - High-performance BRAVO algorithm implementation | ||
| - Optimized fast-path for readers (lock-free in common case) | ||
| - Prevents writer starvation with adaptive policy | ||
| - More complex but potentially much faster | ||
|
|
||
| ### Cache Operation Breakdown | ||
|
|
||
| #### Read-Only Operations (can use shared locks): | ||
| ``` | ||
| Cache.cc:345 - stripe->directory.probe() [cache lookup] | ||
|
Contributor
There was a problem hiding this comment. I tried this approach before and faced a problem. If I understand correctly, Directory::probe needs the write lock when it found invalid Dir and call dir_delete_entry. This is why I'm waiting RCU/Hazard Pointer. trafficserver/src/iocore/cache/CacheDir.cc Lines 522 to 527 in 0411aa7 |
||
| Cache.cc:548 - stripe->directory.probe() [cache lookup] | ||
| Cache.cc:691 - stripe->directory.probe() [cache lookup] | ||
| CacheRead.cc:479 - stripe->directory.probe() [directory scan] | ||
| CacheRead.cc:488 - stripe->directory.probe() [fragment check] | ||
| CacheRead.cc:705 - stripe->directory.probe() [directory scan] | ||
| CacheRead.cc:715 - stripe->directory.probe() [fragment check] | ||
| CacheRead.cc:815 - stripe->directory.probe() [cache lookup] | ||
| CacheRead.cc:1141 - stripe->directory.probe() [cache lookup] | ||
| CacheWrite.cc:650 - stripe->directory.probe() [collision check] | ||
| CacheWrite.cc:741 - stripe->directory.probe() [collision check] | ||
| ``` | ||
|
|
||
| #### Write Operations (need exclusive locks): | ||
| ``` | ||
| CacheWrite.cc:100 - stripe->directory.remove() [directory modify] | ||
| CacheWrite.cc:282 - stripe->directory.remove() [directory modify] | ||
| CacheWrite.cc:340 - stripe->directory.insert() [directory modify] | ||
| CacheWrite.cc:347 - stripe->directory.insert() [directory modify] | ||
| CacheWrite.cc:424 - stripe->directory.insert() [directory modify] | ||
| CacheWrite.cc:520 - stripe->directory.insert() [directory modify] | ||
| CacheRead.cc:518 - stripe->directory.remove() [directory modify] | ||
| CacheRead.cc:654 - stripe->directory.remove() [directory modify] | ||
| CacheRead.cc:735 - stripe->directory.remove() [directory modify] | ||
| CacheRead.cc:791 - stripe->directory.remove() [directory modify] | ||
| ``` | ||
|
|
||
| ### Expected Performance Impact | ||
|
|
||
| With 1000 concurrent clients: | ||
|
|
||
| **Current Behavior (Exclusive Locks)**: | ||
| - Only 1 thread can hold the stripe lock at a time | ||
| - Read operations block other reads unnecessarily | ||
| - Lock contention causes 42ms average delay | ||
| - Throughput: ~17,520 req/s | ||
|
|
||
| **With Reader/Writer Locks**: | ||
| - Multiple readers can hold shared lock simultaneously | ||
| - Only writes need exclusive access | ||
| - For non-cacheable content (100% cache misses), most operations are **reads** (directory probes) | ||
| - Potential throughput improvement: **10-100x** for read-heavy workloads | ||
|
|
||
| ### Example Benchmark Scenario | ||
|
|
||
| Your benchmark: 1M requests, 1K clients, non-cacheable origin | ||
|
|
||
| **Operation mix**: | ||
| - `directory.probe()` for cache lookup: **READ** | ||
| - `directory.probe()` for collision check: **READ** | ||
| - No cache writes (non-cacheable content) | ||
|
|
||
| **Current**: All 1000 clients serialize on exclusive lock | ||
| **With R/W locks**: All 1000 clients can probe directory concurrently | ||
|
|
||
| ### Implementation Strategy | ||
|
|
||
| #### Option 1: Drop-in Replacement with `ts::shared_mutex` | ||
| ```cpp | ||
| // In StripeSM.h, change stripe mutex type | ||
| class StripeSM { | ||
| // OLD: | ||
| // Ptr<ProxyMutex> mutex; | ||
|
|
||
| // NEW: | ||
| ts::shared_mutex stripe_mutex; | ||
|
|
||
| // Keep ProxyMutex for compatibility with event system | ||
| Ptr<ProxyMutex> mutex; | ||
| }; | ||
| ``` | ||
| #### Option 2: Selective Lock Types | ||
| Use shared locks for probes: | ||
| ```cpp | ||
| // For reads (Cache.cc:344) | ||
| CACHE_TRY_LOCK_SHARED(lock, stripe->mutex, mutex->thread_holding); | ||
| if (!lock.is_locked()) { retry... } | ||
| stripe->directory.probe(key, stripe, &result, &last_collision); | ||
| // For writes (CacheWrite.cc:340) | ||
| CACHE_TRY_LOCK_EXCLUSIVE(lock, stripe->mutex, mutex->thread_holding); | ||
| if (!lock.is_locked()) { retry... } | ||
| stripe->directory.insert(&first_key, stripe, &dir); | ||
| ``` | ||
|
|
||
| #### Option 3: Use BRAVO for Maximum Performance | ||
| ```cpp | ||
| // In StripeSM.h | ||
| ts::bravo::shared_mutex stripe_mutex; | ||
|
|
||
| // For reads with token tracking | ||
| ts::bravo::Token token; | ||
| ts::bravo::shared_lock<ts::bravo::shared_mutex> lock(stripe_mutex, token); | ||
| ``` | ||
| ## Challenges and Considerations | ||
| ### 1. Integration with ProxyMutex/Event System | ||
| - ATS uses `ProxyMutex` with the event system | ||
| - Reader/writer locks need integration with continuation scheduling | ||
| - May need hybrid approach: keep `ProxyMutex` for event handling, add R/W lock for directory | ||
| ### 2. Lock Ordering | ||
| - Must prevent deadlocks | ||
| - Document lock acquisition order | ||
| - Use consistent `try_lock` vs blocking patterns | ||
| ### 3. Writer Starvation | ||
| - `ts::shared_mutex` already prevents this | ||
| - `ts::bravo::shared_mutex` has adaptive policy | ||
| - Not a major concern with available implementations | ||
| ### 4. Testing Requirements | ||
| - Ensure correctness under high concurrency | ||
| - Verify no performance regression for write-heavy workloads | ||
| - Benchmark read-heavy vs write-heavy vs mixed workloads | ||
| ## Recommendation | ||
| **Use `ts::shared_mutex` as a starting point**: | ||
| 1. **Lowest risk**: Standard pthread-based implementation | ||
| 2. **Clear semantics**: Writer-starvation prevention built-in | ||
| 3. **Easy migration**: Similar API to existing mutexes | ||
| 4. **Proven correctness**: Well-tested implementation | ||
| **Migration path**: | ||
| 1. Add R/W lock to `StripeSM` alongside existing mutex | ||
| 2. Convert directory probes to use shared locks | ||
| 3. Keep exclusive locks for modifications | ||
| 4. Benchmark and measure contention reduction | ||
| 5. If needed, upgrade to `ts::bravo::shared_mutex` for better performance | ||
| ## Expected Outcome | ||
| For your specific benchmark (non-cacheable content): | ||
| - **Before**: 17,520 req/s, 55.94ms latency (42ms from lock contention) | ||
| - **After**: 50,000-70,000 req/s, ~15ms latency (close to cache-disabled performance) | ||
| **Improvement**: ~3-4x throughput, ~4x lower latency | ||
| ## Implementation Estimate | ||
| - **Small change**: Replace probe operations with shared locks (~100 lines) | ||
| - **Medium change**: Add R/W lock to StripeSM, convert all operations (~500 lines) | ||
| - **Testing**: High-concurrency stress tests, correctness verification | ||
| This is a **high-impact, moderate-effort** optimization that directly addresses the root cause of cache lock contention. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,144 @@ | ||
| # Cache Reader/Writer Lock Implementation - Benchmark Results | ||
|
|
||
| ## Implementation Summary | ||
|
|
||
| ### Changes Made | ||
|
|
||
| 1. **Added `ts::shared_mutex` to `StripeSM`** (`src/iocore/cache/StripeSM.h`) | ||
| - New member: `ts::shared_mutex dir_mutex` | ||
| - Used for directory operations to reduce lock contention | ||
|
|
||
| 2. **Created RAII Lock Wrappers** (`src/iocore/cache/P_CacheInternal.h`) | ||
| - `CacheDirSharedLock` - for read operations (directory probes) | ||
| - `CacheDirExclusiveLock` - for write operations (directory insert/remove) | ||
| - Macros: `CACHE_DIR_TRY_LOCK_SHARED` and `CACHE_DIR_TRY_LOCK_EXCLUSIVE` | ||
|
|
||
| 3. **Converted Critical Read Paths** (`src/iocore/cache/Cache.cc`) | ||
| - `Cache::open_read()` - two variants (lines 344-379, 549-588) | ||
| - `Cache::open_write()` - initial probe for updates (lines 684-723) | ||
| - All `directory.probe()` calls now use shared locks | ||
| - Multiple concurrent readers can access directory simultaneously | ||
|
|
||
| ## Benchmark Results | ||
|
|
||
| ### Test Configuration | ||
| - **Workload**: 1,000,000 requests with 1,000 concurrent clients | ||
| - **Origin**: Non-cacheable responses (100% cache misses) | ||
| - **Protocol**: HTTP/1.1 | ||
| - **ATS Config**: Cache enabled, default settings | ||
|
|
||
| ### Performance Comparison | ||
|
|
||
| | Metric | Baseline (Exclusive Locks) | With Reader/Writer Locks | Improvement | | ||
| |--------|---------------------------|--------------------------|-------------| | ||
| | **Throughput** | 17,520 req/s | 44,218 req/s | **+152% (2.5x)** | | ||
| | **Mean Latency** | 55.94ms | 22.23ms | **-60% (2.5x faster)** | | ||
| | **Cache Overhead** | 42.81ms | 9.10ms | **-79%** | | ||
|
|
||
| ### Detailed Metrics | ||
|
|
||
| #### Baseline (Before R/W Locks) | ||
| ``` | ||
| finished in 57.06s, 17,520.23 req/s, 7.62MB/s | ||
| time for request: | ||
| min: 1.78ms max: 278.30ms mean: 55.94ms sd: 10.94ms | ||
| ``` | ||
|
|
||
| #### With Reader/Writer Locks (After) | ||
| ``` | ||
| finished in 22.62s, 44,218.34 req/s, 19.23MB/s | ||
| time for request: | ||
| min: 604us max: 136.93ms mean: 22.23ms sd: 4.33ms | ||
| ``` | ||
|
|
||
| #### Cache Disabled (Reference - Upper Bound) | ||
| ``` | ||
| finished in 13.31s, 75,130.03 req/s, 32.68MB/s | ||
| time for request: | ||
| min: 437us max: 149.99ms mean: 13.13ms sd: 9.27ms | ||
| ``` | ||
|
|
||
| ## Analysis | ||
|
|
||
| ### Why the Improvement? | ||
|
|
||
| **Before (Exclusive Locks)**: | ||
| - Every cache lookup acquired exclusive stripe mutex | ||
| - 1000 clients → 1000 serialized lock acquisitions | ||
| - Each failed lock = 2ms retry delay | ||
| - Average ~21 retries per request = 42ms overhead | ||
|
|
||
| **After (Reader/Writer Locks)**: | ||
| - Cache lookups (`directory.probe()`) use **shared locks** | ||
| - Multiple readers can hold lock simultaneously | ||
| - Lock contention drastically reduced | ||
| - Retry delays minimized | ||
|
|
||
| ### Remaining Gap to Cache-Disabled Performance | ||
|
|
||
| Current: 44,218 req/s → Target: 75,130 req/s (40% gap remaining) | ||
|
|
||
| **Reasons**: | ||
| 1. **Partial implementation**: Only converted `Cache.cc` read paths | ||
| - `CacheRead.cc` still uses exclusive locks in many places | ||
| - `CacheWrite.cc` write paths not yet converted to exclusive R/W locks | ||
|
|
||
| 2. **Still using ProxyMutex**: Retained `stripe->mutex` for continuation handling | ||
| - Both locks must be acquired (stripe mutex + dir_mutex) | ||
| - Could be optimized further | ||
|
|
||
| 3. **Lock acquisition overhead**: Even shared locks have some cost | ||
| - `try_lock_shared()` is not free | ||
| - Multiple lock checks in critical path | ||
|
|
||
| ### Next Steps for Further Optimization | ||
|
|
||
| 1. **Complete R/W Lock Conversion**: | ||
| - Convert `CacheRead.cc` probe operations to shared locks | ||
| - Convert `CacheWrite.cc` insert/remove to exclusive R/W locks | ||
| - Estimated gain: +10-20% additional throughput | ||
|
|
||
| 2. **Optimize Lock Strategy**: | ||
| - Consider removing dual-lock requirement (ProxyMutex + dir_mutex) | ||
| - Integrate R/W lock with continuation scheduling | ||
| - Estimated gain: +5-10% throughput | ||
|
|
||
| 3. **Consider BRAVO Locks** (`ts::bravo::shared_mutex`): | ||
| - Lock-free fast path for readers under low contention | ||
| - Better performance for read-heavy workloads | ||
| - Estimated gain: +10-15% throughput | ||
|
|
||
| 4. **Profile Remaining Bottlenecks**: | ||
| - Identify other serialization points | ||
| - Check for unnecessary synchronization | ||
|
|
||
| ## Conclusion | ||
|
|
||
| **Reader/writer locks successfully reduced cache lock contention by 2.5x**, bringing performance from **17,520 req/s to 44,218 req/s** for non-cacheable content. | ||
|
|
||
| This is a **partial implementation** covering only the most critical read paths. With full implementation and further optimization, we expect to approach or match the cache-disabled performance of **75,130 req/s**. | ||
|
|
||
| ### Production Readiness | ||
|
|
||
| **Status**: Prototype - needs additional work | ||
|
|
||
| **Required before production**: | ||
| 1. Complete conversion of all cache read/write paths | ||
| 2. Extensive testing under varied workloads | ||
| 3. Verify correctness with write-heavy scenarios | ||
| 4. Performance testing with cacheable content | ||
| 5. Load testing with mixed read/write workloads | ||
|
|
||
| **Risk Assessment**: Low-Medium | ||
| - R/W locks are a well-understood pattern | ||
| - `ts::shared_mutex` is already used elsewhere in ATS | ||
| - Main risk is incomplete conversion leading to edge cases | ||
|
|
||
| ## Files Modified | ||
|
|
||
| 1. `src/iocore/cache/StripeSM.h` - Added `dir_mutex` member | ||
| 2. `src/iocore/cache/P_CacheInternal.h` - Added lock wrapper classes and macros | ||
| 3. `src/iocore/cache/Cache.cc` - Converted 3 critical read paths to use shared locks | ||
|
|
||
| **Total lines changed**: ~100 lines | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lock contention problem of Stripe Mutex was main motivation of introducing BRAVO.