[dbnode] Adaptive WriteBatch allocations #3429
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3429 +/- ##
=========================================
+ Coverage 72.1% 72.3% +0.2%
=========================================
Files 1100 1100
Lines 103565 102612 -953
=========================================
- Hits 74734 74268 -466
+ Misses 23664 23234 -430
+ Partials 5167 5110 -57
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
| // defaultWritePoolMaxBatchSize is the default maximum size for a writeBatch that the pool | ||
| // will allow to remain in the pool. Any batches larger than that will be discarded to prevent | ||
| // excessive memory use forever in the case of an exceptionally large batch write. | ||
| defaultMaxBatchSize = 100000 | ||
| defaultMaxBatchSize = 10240 |
There was a problem hiding this comment.
Reducing this default as 100000 seems to be crazy lot (it would result in a 39MB batch).
linasm
changed the title
src/dbnode/ts/writes/write_batch.go
Outdated
| if batchSize > cap(b.writes) { | ||
| writes = make([]BatchWrite, 0, batchSize) | ||
| batchCap := batchSize | ||
| if cap(b.writes) == 0 { |
There was a problem hiding this comment.
Why do this if cap of writes is zero?
I thought you’d always want to allocate 1.2x the size you actually need since next request will likely have a little more or a little less.
There was a problem hiding this comment.
The reason for this was to preserve the original behaviour when having InitialBatchSize set in config (so that if you have initial batch size of 128 in config, and a batch size of 200 arrives, exactly 200 would be allocated, and not 200*1.2).
I'll add an explicit writeBatch.adaptiveSize flag which will help me handle every case better.
| if b.adaptiveSize { | ||
| batchCap = adaptiveBatchCap | ||
| } | ||
| b.writes = make([]BatchWrite, 0, batchCap) |
There was a problem hiding this comment.
nit: I think it would be very useful to have a gauge for current allocated batch capacity. It could be valuable in cases when, e.g. we receive one huge batch and subsequent batches are much smaller. The memory usage will probably be increased after this even if we are receiving smaller batches so this gauge could help us to identify such cases.
There was a problem hiding this comment.
How would this gauge work? I guess we would increment it on line 139, but when would we decrement it?
There was a problem hiding this comment.
It could be also a counter for now, I was thinking that maybe in future we will update this to also shrink batch capacity so gauge makes sense from this point of view.
* master: [dbnode] Adaptive WriteBatch allocations (#3429)
What this PR does / why we need it:
In some cases we have to handle write batches that are an order of magnitude smaller than the default expected size (128).
Thus, always preallocating the default size (which results in 49152 bytes allocated) is really wasteful.
This PR attempts to avoid preallocating write batches of the given size, and instead defers allocation to a later time, when the actual batch size is known. Then slices of the needed capacity (plus 20% more) are allocated. Allocating 20% more than needed for the current write batch should help avoid subsequent allocations when the same write batch is taken from the pool and is used for another write batch of slightly bigger size.
Special notes for your reviewer:
The original behaviour is preserved in case
InitialBatchSizeis defined in the config.Does this PR introduce a user-facing and/or backwards incompatible change?:
NONE
Does this PR require updating code package or user-facing documentation?:
NONE