GODRIVER-2677 Improve memory pooling. #1157
Conversation
There was a problem hiding this comment.
Nice, simple design. Given benchmark results, seems like a solid change to me 🧑🔧 . Just a few questions.
Need to look closer at the benchmarks, seems more nuanced after discussing offline with @qingyang-hu .
x/mongo/driver/byteslicepool.go
Outdated
| // Proper usage of a sync.Pool requires each entry to have approximately the same memory | ||
| // cost. To obtain this property when the stored type contains a variably-sized buffer, | ||
| // we add a hard limit on the maximum buffer to place back in the pool. We limit the | ||
| // size to 16MiB because that's the maximum wire message size supported by MongoDB. |
There was a problem hiding this comment.
| // size to 16MiB because that's the maximum wire message size supported by MongoDB. | |
| // size to 16MB because that's the maximum BSON document size supported by MongoDB. |
[nit] The size limitation you're referring to is 16MB (not mebibytes abbreviated with MiB) and it's not a limitation on the wire message size but on the document size.
x/mongo/driver/byteslicepool.go
Outdated
| ) | ||
|
|
||
| type byteslicePool struct { | ||
| pool interface { |
There was a problem hiding this comment.
Why not just make this *sync.Pool?
There was a problem hiding this comment.
It is mostly for the convenience of testing.
x/mongo/driver/byteslicepool.go
Outdated
|
|
||
| countmax int | ||
| count int | ||
| mutex *sync.Mutex |
There was a problem hiding this comment.
| mutex *sync.Mutex | |
| // mutex guards countmax and count. | |
| mutex *sync.Mutex |
[opt] we usually have comments describing the function of mutexes.
x/mongo/driver/byteslicepool.go
Outdated
| defer p.mutex.Unlock() | ||
| if p.count < p.countmax { | ||
| p.count++ | ||
| return (*p.pool.Get().(*[]byte))[:0] |
There was a problem hiding this comment.
The global count approach suffers from two issues:
pool.Get()doesn't guarantee that it will always return an element from the pool (it may callNewinstead). As a result, thep.countvalue may not actually reflect the number of buffers "checked out" and may eventually diverge significantly from the actual number of buffers held, leading to unexpected behavior.sync.Poolis optimized to prevent locking whenever possible. Holding a global lock defeats that optimization and adds a global contention point to running operations.
Issues with sync.Pool usage leading to high memory usage very similar to the ones we observed in the Go driver are discussed in golang/go#23199 and golang/go#27735. We should consider the approach suggested in golang/go#27735 (comment) which works with the non-deterministic behavior of sync.Pool and preserves its performance characteristics.
Another solution to the same problem is implemented in the "encoding/json" library, which keeps different buffer pools for different size buffers (see this PR). That approach seems to require significantly more code and is much harder to understand, so I don't recommend it.
benjirewis
requested review from
benjirewis
and removed request for
benjirewis
qingyang-hu
force-pushed
the
godriver2677
branch
from
044a52c to
9bdf0a2
Compare
qingyang-hu
changed the title
| // Recycle byte slices that are smaller than 16MiB and at least half occupied. | ||
| if c := cap(*wm); c < 16*1024*1024 && c/2 < len(*wm) { | ||
| memoryPool.Put(wm) | ||
| } |
There was a problem hiding this comment.
Actually, if I recycle the low-occupied slices as well, the benchmark shows an even lower memory consumption.
benchmark/operation_test.go
Outdated
| @@ -8,7 +8,9 @@ package benchmark | |||
|
|
|||
| import ( | |||
| "context" | |||
| "math/rand" | |||
There was a problem hiding this comment.
crypto/rand is the secure way to generate random numbers. It probably doesn't matter in a test case, but it could be good to enforce using crypto/rand over math/rand in all use cases in the project. What are your thoughts? We currently skip this linter check on _test.go files.
benchmark/operation_test.go
Outdated
| if random.Int()%2 == 0 { | ||
| t = text | ||
| } else { | ||
| t = "hello" | ||
| } |
There was a problem hiding this comment.
The t variable is kind of hard to read, can we just change the value of text on even random numbers?
| if random.Int()%2 == 0 { | |
| t = text | |
| } else { | |
| t = "hello" | |
| } | |
| if random.Int()%2 == 0 { | |
| text = "hello" | |
| } |
benchmark/operation_test.go
Outdated
| if random.Int()%2 == 0 { | ||
| t = text | ||
| } else { | ||
| t = "hello" | ||
| } |
There was a problem hiding this comment.
The t variable is kind of hard to read, can we just change the value of text on even random numbers?
| if random.Int()%2 == 0 { | |
| t = text | |
| } else { | |
| t = "hello" | |
| } | |
| if random.Int()%2 == 0 { | |
| text = "hello" | |
| } |
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| gotWM, _, gotErr := Operation{}.roundTrip(context.Background(), tc.conn, tc.paramWM) |
There was a problem hiding this comment.
This test is mostly for testing whether the input slice is returned, in my opinion. Since we no longer reuse the slice, the test is redundant.
x/mongo/driver/operation.go
Outdated
| @@ -855,18 +856,18 @@ func (op Operation) retryable(desc description.Server) bool { | |||
|
|
|||
| // roundTrip writes a wiremessage to the connection and then reads a wiremessage. The wm parameter | |||
| // is reused when reading the wiremessage. | |||
| func (op Operation) roundTrip(ctx context.Context, conn Connection, wm []byte) (result, pooledSlice []byte, err error) { | |||
| func (op Operation) roundTrip(ctx context.Context, conn Connection, wm []byte) (result []byte, err error) { | |||
There was a problem hiding this comment.
What are your thoughts on the named return parameters? A lot of these functions are small and the documentation on them is fairly clear. IMO we would remove them.
There was a problem hiding this comment.
I agree, at least for this one. result is not even used.
benchmark/operation_test.go
Outdated
| @@ -48,7 +53,11 @@ func BenchmarkClientWrite(b *testing.B) { | |||
| b.ResetTimer() | |||
| b.RunParallel(func(p *testing.PB) { | |||
| for p.Next() { | |||
| _, err := coll.InsertOne(context.Background(), bson.D{{"text", text}}) | |||
| n, err := rand.Int(rand.Reader, big.NewInt(int64(len(teststrings)))) | |||
There was a problem hiding this comment.
Reading from crypto/rand may be intermittently slow on some systems and may negatively impact the reliability of the benchmark. If the goal is to get a consistent distribution of small and large messages, consider incrementing a counter and selecting teststrings[i % len(teststrings)] instead of using a random number.
benchmark/operation_test.go
Outdated
| } | ||
|
|
||
| b.ResetTimer() | ||
| b.RunParallel(func(p *testing.PB) { | ||
| for p.Next() { | ||
| n, err := rand.Int(rand.Reader, big.NewInt(2)) |
There was a problem hiding this comment.
Same as above: Consider incrementing a counter and using teststrings[i % len(teststrings)] instead of using a random number.
GODRIVER-2677
Summary
Reduce high memory consumption introduced by GODRIVER-2021.
Background & Motivation
I limited 512 slices in the pool.Get()allocates directly from the system when it reaches the limit. So the size of the pool shall not be bigger than 16MB * 512 ~= 8GB in theory considering the ticket indicates ~20GB of memory consumption.I did not reset the capacity of returned byte slice because it would cause more allocations.2nd attempt:
New benchstat from the new
benchmark/operation_test.go:Notes:
ClientWrite/not_compressed-10can be eliminated if I recycle the slices regardless of their occupation. However, I decided to adopt a less aggressive policy of pooling highly-occupied slices only to avoid holding slices with large sizes.