Reduce allocations for logfile reader#40796
Conversation
Before this change, the log decoder function provided by the log driver to logfile would not be able to re-use buffers, causing undeeded allocations and memory bloat for dockerd. This change introduces an interface that allows the log driver to manage it's memory usge more effectively. This only affects json-file and local log drivers. `json-file` still is not great just because of how the json decoder in the stdlib works. `local` is significantly improved. Signed-off-by: Brian Goff <[email protected]>
| jl.Attrs = make(map[string]string) | ||
| for k := range jl.Attrs { | ||
| delete(jl.Attrs, k) | ||
| } |
There was a problem hiding this comment.
Is the explicit delete better than assigning an empty map? From what I understood it's the same thing.
There was a problem hiding this comment.
In one case it has to allocate, in the other not.
Also this pattern for deleting the map is optimized by the compiler.
There was a problem hiding this comment.
It's worth noting that right now I believe map elements that are deleted won't actually be GCed until the map itself is GCed, see golang/go#20135
There was a problem hiding this comment.
so would the old approach be better? should we manually trigger a garbage collect?
There was a problem hiding this comment.
Realistically, this is not going to change in size. We reset it because it's proper b/c things could change, but in reality it is not changing.
There was a problem hiding this comment.
Without doing some profiling I'm really not sure. How often are these reset vs. created/destroyed? Maybe the higher memory footprint is worth it's rather small and we avoid an expensive allocation, or maybe we'd rather take one more allocation to avoid the higher memory footprint if its significant.
There was a problem hiding this comment.
I do not think there is any higher memory footprint because these attrs actually never change.
|
@kolyshkin looks like your questions were answered; anything left? @SamWhited PTAL |
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
I'll copy @SamWhited's LGTM
5 participants
Before this change, the log decoder function provided by the log driver
to logfile would not be able to re-use buffers, causing undeeded
allocations and memory bloat for dockerd.
This change introduces an interface that allows the log driver to manage
it's memory usge more effectively.
This only affects json-file and local log drivers.
json-filestill is not great just because of how the json decoder in thestdlib works.
localis significantly improved.Relates to docker/for-linux#848
Fixes #39963