gzip: fix memory allocs (buffers not returned to pool) #28
Conversation
Fix allocation leaks during gzip writes. Due to incorrect use of the dstPool (unmatched Get and Put), large amounts of memory were temporarily allocated during writes and not put back into the pool. This also removes some special handling code in compressCurrent that would recursively call itself for too large input buffers. This condition can never occur, because Write ensures that blocks are capped and there is no other public interface that extends currentBuffer. The recursive call that slices the buffer would have made returning byte slices to the Pool dangerous, as we could have been returning the same underlying buffer multiple times. This also adds a test to check allocations per Write to prevent regressions. There is further room for improvement, but this was by far the biggest leak.
|
Thanks. I will review tomorrow. Please take out the go.mod - I have not decided to go that route yet. |
wojas
force-pushed
the
fix-memory-allocs
branch
from
8d889e8 to
c548e59
Compare
|
Thanks for the fast response. I took out the |
|
I found that the tests pass without |
The allocation test was incorrectly using Alloc (the current number of allocated bytes) instead of TotalAlloc (the cummulative number of bytes), which would break the test if garbage collection happened during the run. This also increases the allocated bytes check to allow for occasional other background allocations. I once saw 3kB instead of the usual <700B allocations. This is still sufficiently low to detect pool usage issues.
In the panics, make clear that these can only happen due to concurrent Write call races, which are unsafe. The alternative would be to silently corrupt data. Move the panic to the beginning of compressCurrent to not deadlock tests after a panic.
Use a pool buffer for prevTail instead of slicing the current buffer, which may have been returned to the pool by the time the tail is accessed.
Skip the allocation tests when running with the race detector, because the race detector allocates a lot of memory. Rename the unreliable tests file.
|
This PR should be done as is. The remaining test failures are also present in master and I cannot reproduce them locally in my branch, which is based on v1.2.1. I suspect they are caused by a8ba214. |
|
Thanks! I'll take a look. 👍 |
| // but it's better than the 175846 bytes before the pool release fix this tests. | ||
| // TODO: Further reduce allocations | ||
| if allocBytes > 10240 { | ||
| t.Errorf("Write allocated too much memory per run (%.0f bytes), Pool used incorrectly?", allocBytes) |
There was a problem hiding this comment.
For the failures it reports gzip_test.go:637: Write allocated too much memory per run (18446744073709436 bytes), Pool used incorrectly?
That is 18446.7TB - I think you would be hard pressed to allocated that much, so I suspect there is something wrong.
There was a problem hiding this comment.
Are you sure this happened with the latest version? I noticed this in earlier test runs and 799eba1 fixes this. Before that commit I was using .Alloc instead of .TotalAlloc.
From the MemStats comments:
TotalAlloc increases as heap objects are allocated, but unlike Alloc and HeapAlloc, it does not decrease when objects are freed.
There was a problem hiding this comment.
Yeah, seems I mixed up the runs. Anyway, merging master should fix the writer.
|
|
||
| // Read the final statistics | ||
| runtime.ReadMemStats(&memstats) | ||
| allocs := memstats.TotalAlloc - oldTotal |
There was a problem hiding this comment.
I suspect the error is caused by memstats.TotalAlloc > oldTotal causing an underflow and the crazy numbers.
Not really an expert on how that can happen.
| w.SetConcurrency(1000, 1) | ||
| data := bytes.Repeat([]byte("T"), 100000) // varying block splits | ||
|
|
||
| const n = 1000 |
There was a problem hiding this comment.
Maybe:
| const n = 1000 | |
| n := 1000 | |
| if testing.Short() { | |
| n = 10 | |
| } |
(or make the i++ change to bigger increments).
Though you have this guarded nicely by tags.
There was a problem hiding this comment.
This test tries to reproduce a race condition. I tried, but I cannot reproduce it with n = 10.
Co-Authored-By: Klaus Post <[email protected]>
wojas
force-pushed
the
fix-memory-allocs
branch
from
a13a592 to
6b5bd9d
Compare
|
Cool. I will give it some tests before the next release. Thanks a bunch! |
Fix allocation leaks during gzip writes. Due to incorrect use of the dstPool (unmatched Get and Put), large amounts of memory were temporarily allocated during writes and not put back into the pool.
This also removes some special handling code in compressCurrent that would recursively call itself for too large input buffers. This condition can never occur, because Write ensures that blocks are capped
and there is no other public interface that extends currentBuffer. The recursive call that slices the buffer would have made returning byte slices to the Pool dangerous, as we could have been returning the same
underlying buffer multiple times.
This also adds a test to check allocations per Write to prevent regressions. There is further room for improvement, but this was by far the biggest leak.
Closes #8
Additionally, this adds a go.mod for Go modules support.(Note that tests broke with recent commit a8ba214).