Remove redundant Mutex from SharedMemoryReservation#21676
Remove redundant Mutex from SharedMemoryReservation#21676Dandandan wants to merge 1 commit intoapache:mainfrom
Conversation
`MemoryReservation` is already internally thread-safe: its `size` field uses `AtomicUsize`, and the underlying `MemoryPool` implementations (`GreedyMemoryPool`, `FairSpillPool`, `UnboundedMemoryPool`) handle their own locking. The outer `Mutex` wrapper in `SharedMemoryReservation` was therefore redundant and caused unnecessary lock contention. In the repartition operator, this lock was acquired on every batch by every input partition (sender) plus the output partition (receiver), making it a serialization bottleneck that reduced CPU utilization during parallel query execution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
run benchmarks |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing remove-shared-memory-reservation-mutex (3bf05f8) to 5c653be (merge-base) diff using: tpcds File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing remove-shared-memory-reservation-mutex (3bf05f8) to 5c653be (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing remove-shared-memory-reservation-mutex (3bf05f8) to 5c653be (merge-base) diff using: tpch File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
|
run benchmark tpch_mem |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing remove-shared-memory-reservation-mutex (3bf05f8) to 5c653be (merge-base) diff using: tpch_mem File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpch_mem — base (merge-base)
tpch_mem — branch
File an issue against this benchmark runner |
Which issue does this PR close?
Rationale for this change
SharedMemoryReservationwas defined asArc<Mutex<MemoryReservation>>, butMemoryReservationis already internally thread-safe:sizefield usesAtomicUsizewith atomicfetch_update/fetch_add/fetch_subMemoryPoolimplementations (GreedyMemoryPool,FairSpillPool,UnboundedMemoryPool) all handle their own internal locking/atomicsThe outer
Mutexwas therefore redundant and caused unnecessary lock contention. In the repartition operator specifically, this lock was acquired on every batch by every input partition (sender) plus the output partition (receiver), making it a serialization bottleneck that reduced CPU utilization during parallel query execution.What changes are included in this PR?
SharedMemoryReservationtype alias fromArc<Mutex<MemoryReservation>>toArc<MemoryReservation>.lock()calls at all usage sites (repartition operator and symmetric hash join)parking_lot::MuteximportsAre these changes tested?
Covered by existing tests — no behavioral change, only removal of redundant synchronization. All existing repartition and symmetric hash join tests continue to pass.
Are there any user-facing changes?
No.
SharedMemoryReservationispub(crate).🤖 Generated with Claude Code