masaori335 commented on code in PR #12601:
URL: https://github.com/apache/trafficserver/pull/12601#discussion_r2453591345
##########
CACHE_RWLOCK_ANALYSIS.md:
##########
@@ -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]
+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);
Review 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`.
https://github.com/apache/trafficserver/blob/0411aa7bdb7139b61bc4671232edcabdb097b941/src/iocore/cache/CacheDir.cc#L522-L527
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
