custom-buffer-pool analysis - #35
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a custom sharded, memory-mapped buffer pool (gomem) to optimize gRPC client performance under high concurrency, along with associated benchmarks, tests, and documentation. Feedback on these changes highlights critical issues, including the need for platform-specific build constraints to prevent compilation failures on unsupported platforms, and a potential race condition leading to resource leaks during concurrent arena allocations. Additionally, improvements are suggested to avoid pool pollution in SimpleBufferPool by discarding too-small buffers, optimize the double-free protection scan to reduce lock contention, and prevent slice capacity degradation during slot compaction.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| @@ -0,0 +1,568 @@ | |||
| // Copyright 2026 Google LLC | |||
There was a problem hiding this comment.
The custom memory-mapped buffer pool uses platform-specific assembly/syscalls (such as syscall.RawSyscall(309, ...) for SYS_GETCPU on Linux x86_64) and unexported runtime linknames (runtime.procPin). Without build tags, this file will fail to compile or crash on non-Linux or non-x86_64 platforms (e.g., macOS, Windows, ARM64). Add a build constraint to restrict this file to supported platforms.
| // Copyright 2026 Google LLC | |
| //go:build linux && amd64 | |
| // Copyright 2026 Google LLC |
| if ok { | ||
| p.pool.Put(bs) | ||
| } |
There was a problem hiding this comment.
In SimpleBufferPool.Get, when a buffer retrieved from the pool is too small (cap(*bs) < size), putting it back into the pool (p.pool.Put(bs)) causes pool pollution. In subsequent calls, p.pool.Get() will repeatedly return this same too-small buffer, reject it, put it back, and allocate a new one on the heap. This leads to an infinite loop of pool pollution and forces constant heap allocations for larger requests. Discard the too-small buffer instead so it can be garbage collected.
| _ = procPin() | ||
| } | ||
|
|
||
| if len(shard.freeSlots)-shard.head == 0 { |
There was a problem hiding this comment.
When a shard runs out of slots, the lock is released (shard.mu.Unlock()) and the goroutine unpins to perform rawMmap safely. Under high concurrency, multiple goroutines can concurrently find the shard empty, release the lock, allocate a new arena, and then blindly append it to the shard without double-checking if another thread already allocated an arena. This leads to a massive over-allocation of arenas (resource leak/bloat) under high load.
To fix this, double-check len(shard.freeSlots) - shard.head == 0 after re-acquiring the lock. If it is no longer empty, either free the newly allocated arena or keep it in a spare list instead of blindly appending it.
| for i := shard.head; i < len(shard.freeSlots); i++ { | ||
| if shard.freeSlots[i] == slotPacked { | ||
| shard.mu.Unlock() | ||
| *bufPtr = nil | ||
| return // Ignore duplicate free | ||
| } | ||
| } |
There was a problem hiding this comment.
The double-free protection performs a linear scan over shard.freeSlots from shard.head to len(shard.freeSlots) under the shard's mutex lock. Under high concurrency and large pool sizes, this Put path and increase lock contention. Consider using a Arena struct) if double-free protection is strictly required, or removing the scan if the caller is guaranteed to be well-behaved.
| if shard.head > 128 { | ||
| shard.freeSlots = shard.freeSlots[shard.head:] | ||
| shard.head = 0 | ||
| } |
There was a problem hiding this comment.
Reslicing shard.freeSlots = shard.freeSlots[shard.head:] shifts the slice's start pointer and decreases its capacity (cap). As head grows and compaction is triggered, the capacity of freeSlots continually shrinks. When new elements are appended later, it will frequently trigger new underlying array allocations and copies. Use copy to shift the elements to the beginning of the slice to preserve the capacity.
| if shard.head > 128 { | |
| shard.freeSlots = shard.freeSlots[shard.head:] | |
| shard.head = 0 | |
| } | |
| if shard.head > 128 { | |
| copy(shard.freeSlots, shard.freeSlots[shard.head:]) | |
| shard.freeSlots = shard.freeSlots[:len(shard.freeSlots)-shard.head] | |
| shard.head = 0 | |
| } |
No description provided.