Fix #33: Low performance due to GC on gzip/zlib writers #35
Conversation
|
Note: the failed build for Go 1.6 and lower is due to a change in |
gelf_writer.go
Outdated
| if w.gzipWriter == nil { | ||
| w.gzipWriter, err = gzip.NewWriterLevel(&zBuf, w.CompressionLevel) | ||
| } | ||
| w.gzipWriter.Reset(&zBuf) |
There was a problem hiding this comment.
if err is different from nil, this will crash since w is nil.
gelf_writer.go
Outdated
| switch w.CompressionType { | ||
| case CompressGzip: | ||
| zw, err = gzip.NewWriterLevel(&zBuf, w.CompressionLevel) | ||
| if w.gzipWriter == nil { |
There was a problem hiding this comment.
Since we have a constructor (NewWriter), I suggest initializing in there instead.
There was a problem hiding this comment.
Actually, no, I just realized Writer is exported, so if someone has the great idea of instantiating it directly, zw becomes unset and unaccessible at the same time.
gelf_writer.go
Outdated
| @@ -30,6 +30,9 @@ type Writer struct { | |||
| Facility string // defaults to current process name | |||
| CompressionLevel int // one of the consts from compress/flate | |||
| CompressionType CompressType | |||
|
|
|||
| gzipWriter *gzip.Writer | |||
There was a problem hiding this comment.
Since these fields are not exported, why don't we keep a single zw (io.WriteCloser) field instead?
There was a problem hiding this comment.
Writer.CompressionType is exported, so to have a single "current writer" field would require checking that the writer type in that field is OK for the current CompressionType value. Something along the lines of changing if w.gzipWriter == nill to if w.zw == nill and w.zwCompressionType == w.CompressionType.
If that is the approach you prefer I'll add it.
There was a problem hiding this comment.
Taking another look at the code, Writer.CompressionLevel is also exported, so that needs to be checked before re-using a writer as well. Since storing a writer for every possible compression level isn't a good idea, I think if w.zw == nill and w.zwCompressionType == w.CompressionType and w.zwCompressionLevel == w.CompressionLevel is what we need.
I tried refactoring bufferedWriter into something that supports a Reset when I created this PR but couldn't get that to work. Since checking for CompressionType and CompressionLevel add more repetitive work (as does the err check you mentioned), I'll give that another go.
gelf_writer.go
Outdated
| if w.zlibWriter == nil { | ||
| w.zlibWriter, err = zlib.NewWriterLevel(&zBuf, w.CompressionLevel) | ||
| } | ||
| w.zlibWriter.Reset(&zBuf) |
|
This API is just awful. Having the writer exported, as well as most of the fields lead us to a lot of conditions and edge cases in the code. We should rewrite this in the future and bump a major version to break the API. Anyway, this is the best we can achieve on that foundation. |
Co-Authored-By: jorygeerts <[email protected]>
Fix for #33