[go] image/png: reduce memory allocs encoding images by reusing buffers
Cezar Espinola (Gerrit)
Cezar Espinola uploaded this change for review.
image/png: reduce memory allocs encoding images by reusing buffers This change greatly reduce memory allocations with little to no impact on performance as we can see on the benchmark results below. Instances of (*png).encoder are now retrieved from a sync.Pool, allowing the use of Reset() on both zlib.Writer and bufio.Writer instances used by the encoder instead of always recreating them. Also, buffers for current and previous rows are saved in the encoder instance and reused as long as their cap() is enough to fit the current image row. benchmark old ns/op new ns/op delta BenchmarkEncodeGray-4 2425337 2324272 -4.17% BenchmarkEncodeNRGBOpaque-4 7384127 7350811 -0.45% BenchmarkEncodeNRGBA-4 8261016 8176085 -1.03% BenchmarkEncodePaletted-4 1843403 1718911 -6.75% BenchmarkEncodeRGBOpaque-4 7436865 7329641 -1.44% BenchmarkEncodeRGBA-4 30476852 30375051 -0.33% benchmark old MB/s new MB/s speedup BenchmarkEncodeGray-4 126.66 132.17 1.04x BenchmarkEncodeNRGBOpaque-4 166.41 167.17 1.00x BenchmarkEncodeNRGBA-4 148.75 150.29 1.01x BenchmarkEncodePaletted-4 166.65 178.72 1.07x BenchmarkEncodeRGBOpaque-4 165.23 167.65 1.01x BenchmarkEncodeRGBA-4 40.32 40.45 1.00x benchmark old allocs new allocs delta BenchmarkEncodeGray-4 32 11 -65.62% BenchmarkEncodeNRGBOpaque-4 32 14 -56.25% BenchmarkEncodeNRGBA-4 32 16 -50.00% BenchmarkEncodePaletted-4 36 12 -66.67% BenchmarkEncodeRGBOpaque-4 32 14 -56.25% BenchmarkEncodeRGBA-4 614432 614418 -0.00% benchmark old bytes new bytes delta BenchmarkEncodeGray-4 852082 1020 -99.88% BenchmarkEncodeNRGBOpaque-4 860146 4520 -99.47% BenchmarkEncodeNRGBA-4 863987 4571 -99.47% BenchmarkEncodePaletted-4 852138 1028 -99.88% BenchmarkEncodeRGBOpaque-4 860147 4520 -99.47% BenchmarkEncodeRGBA-4 3321595 2475856 -25.46% Change-Id: I4488201ae53cb2ad010c68c1e0118ee12beae14e --- M src/image/png/writer.go 1 file changed, 59 insertions(+), 16 deletions(-)
diff --git a/src/image/png/writer.go b/src/image/png/writer.go
index dd87d81..0cd4c31 100644
--- a/src/image/png/writer.go
+++ b/src/image/png/writer.go
@@ -12,6 +12,7 @@
"image/color"
"io"
"strconv"
+ "sync"
)
// Encoder configures encoding PNG images.
@@ -28,6 +29,21 @@
header [8]byte
footer [4]byte
tmp [4 * 256]byte
+ cr [nFilter][]uint8
+ pr []uint8
+ zwl *zwLevel
+ bw *bufio.Writer
+}
+
+type zwLevel struct {
+ zw *zlib.Writer
+ level int
+}
+
+var encoderPool = sync.Pool{
+ New: func() interface{} {
+ return &encoder{}
+ },
}
type CompressionLevel int
@@ -273,12 +289,23 @@
return filter
}
-func writeImage(w io.Writer, m image.Image, cb int, level int) error {
- zw, err := zlib.NewWriterLevel(w, level)
- if err != nil {
- return err
+func zeroMemory(v []uint8) {
+ for i := range v {
+ v[i] = 0
}
- defer zw.Close()
+}
+
+func (e *encoder) writeImage(w io.Writer, m image.Image, cb int, level int) error {
+ if e.zwl == nil || e.zwl.level != level {
+ zw, err := zlib.NewWriterLevel(w, level)
+ if err != nil {
+ return err
+ }
+ e.zwl = &zwLevel{level: level, zw: zw}
+ } else {
+ e.zwl.zw.Reset(w)
+ }
+ defer e.zwl.zw.Close()
bpp := 0 // Bytes per pixel.
@@ -304,12 +331,23 @@
// other PNG filter types. These buffers are allocated once and re-used for each row.
// The +1 is for the per-row filter type, which is at cr[*][0].
b := m.Bounds()
- var cr [nFilter][]uint8
- for i := range cr {
- cr[i] = make([]uint8, 1+bpp*b.Dx())
- cr[i][0] = uint8(i)
+ sz := 1 + bpp*b.Dx()
+ for i := range e.cr {
+ if cap(e.cr[i]) < sz {
+ e.cr[i] = make([]uint8, sz)
+ } else {
+ e.cr[i] = e.cr[i][:sz]
+ }
+ e.cr[i][0] = uint8(i)
}
- pr := make([]uint8, 1+bpp*b.Dx())
+ cr := e.cr
+ if cap(e.pr) < sz {
+ e.pr = make([]uint8, sz)
+ } else {
+ e.pr = e.pr[:sz]
+ zeroMemory(e.pr)
+ }
+ pr := e.pr
gray, _ := m.(*image.Gray)
rgba, _ := m.(*image.RGBA)
@@ -429,7 +467,7 @@
}
// Write the compressed bytes.
- if _, err := zw.Write(cr[f]); err != nil {
+ if _, err := e.zwl.zw.Write(cr[f]); err != nil {
return err
}
@@ -444,13 +482,16 @@
if e.err != nil {
return
}
- var bw *bufio.Writer
- bw = bufio.NewWriterSize(e, 1<<15)
- e.err = writeImage(bw, e.m, e.cb, levelToZlib(e.enc.CompressionLevel))
+ if e.bw == nil {
+ e.bw = bufio.NewWriterSize(e, 1<<15)
+ } else {
+ e.bw.Reset(e)
+ }
+ e.err = e.writeImage(e, e.m, e.cb, levelToZlib(e.enc.CompressionLevel))
if e.err != nil {
return
}
- e.err = bw.Flush()
+ e.err = e.bw.Flush()
}
// This function is required because we want the zero value of
@@ -489,7 +530,9 @@
return FormatError("invalid image size: " + strconv.FormatInt(mw, 10) + "x" + strconv.FormatInt(mh, 10))
}
- var e encoder
+ e := encoderPool.Get().(*encoder)
+ defer encoderPool.Put(e)
+
e.enc = enc
e.w = w
e.m = m
To view, visit this change. To unsubscribe, visit settings.
Brad Fitzpatrick (Gerrit)
Brad Fitzpatrick posted comments on this change.
Patch Set 1:
R=go1.9
We're in a code freeze for the Go 1.8 release at the moment, per https://github.com/golang/go/wiki/Go-Release-Cycle
Somebody can look at this when the tree reopens Feb 1st-ish.
To view, visit this change. To unsubscribe, visit settings.
Cezar Espinola (Gerrit)
Cezar Espinola posted comments on this change.
Patch Set 1:
R=go1.9 We're in a code freeze for the Go 1.8 release at the moment, per https://github.com/golang/go/wiki/Go-Release-Cycle Somebody can look at this when the tree reopens Feb 1st-ish.
No problem, I just remembered about the freeze and should probably have delayed sending this, sorry. I'll be back when 1.9 is ready do start receiving contributions. :)
To view, visit this change. To unsubscribe, visit settings.
Nigel Tao (Gerrit)
Nigel Tao posted comments on this change.
Patch Set 1:
A thought... instead of introducing a new dependency from the image/png package to the sync package, it may be possible to add the buffers to the existing Encoder type, and users of the image/png package can choose to use a sync.Pool, or not.
To view, visit this change. To unsubscribe, visit settings.
Cezar Espinola (Gerrit)
Cezar Espinola uploaded patch set #2 to this change.
image/png: reduce memory allocs encoding images by reusing buffers This change allows greatly reducing memory allocations with a slightly performance improvement as well. Instances of (*png).Encoder can have a optional BufferPool attached to them. This allows reusing temporary buffers used when encoding a new image. This buffers include instances to zlib.Writer and bufio.Writer. Also, buffers for current and previous rows are saved in the encoder instance and reused as long as their cap() is enough to fit the current image row. A new benchmark was added to demonstrate the performance improvement when setting a BufferPool to an Encoder instance: $ go test -bench BenchmarkEncodeGray -benchmem BenchmarkEncodeGray-4 500 2981007 ns/op 103.05 MB/s 852361 B/op 40 allocs/op BenchmarkEncodeGrayWithBufferPool-4 500 2706645 ns/op 113.50 MB/s 1864 B/op 10 allocs/op Change-Id: I4488201ae53cb2ad010c68c1e0118ee12beae14e --- M src/image/png/writer.go M src/image/png/writer_test.go 2 files changed, 103 insertions(+), 16 deletions(-)
To view, visit this change. To unsubscribe, visit settings.
Cezar Espinola (Gerrit)
Cezar Espinola posted comments on this change.
Patch Set 2:
A thought... instead of introducing a new dependency from the image/png package to the sync package, it may be possible to add the buffers to the existing Encoder type, and users of the image/png package can choose to use a sync.Pool, or not.
So, initially I thought about simply adding an `encoder{}` instance to the `Encoder` struct. However, even though the documentation doesn't make any guarantees, calling `(*Encoder).Encode()` is currently goroutine-safe and adding an `encoder{}` to it would break this. That's why a went with the `sync.Pool` approach.
Now, based on your comment I tried another approach to avoid the `sync.Pool` dependency. It's inspired by some similar code that I saw being used in `httputil.ReverseProxy`. Basically a new interface was created to provide the `Get()` and `Put()` methods for reusing buffers and it's possible to set an instance of such interface on the Encoder.
Also, I tried to minimize the amount of changes to the code and ended up aliasing the `encoder` type to make it public instead of renaming it.
Please let me know if this is an acceptable approach. Thanks!
To view, visit this change. To unsubscribe, visit settings.
Nigel Tao (Gerrit)
Nigel Tao posted comments on this change.
Patch Set 2:
(5 comments)
It might be worth renaming Buffer to EncoderBuffer, in case we want a DecoderBuffer in the future.
Yeah, the names are awkward. In hindsight, Encoder should probably be EncodeOptions and Buffer be Encoder, but we're bound by backwards compat.
Patch Set #2, Line 49:
zwl *zwLevelI don't think you need the extra pointer here. Instead, put the two fields in directly:
zw *zlib.Writer zwLevel int
Patch Set #2, Line 308:
if e.zwl == nil || e.zwl.level != level {This becomes
if e.zw == nil || e.zwLevel != level {
Patch Set #2, Line 499:
e.err = e.writeImage(e, e.m, e.cb, levelToZlib(e.enc.CompressionLevel))That first arg should be s/e/e.bw/
It might be worth re-doing the benchmark numbers in the commit message.
File src/image/png/writer_test.go:
Patch Set #2, Line 224:
func BenchmarkEncodeGrayWithBufferPool(b *testing.B) {I'd put this function next to BenchmarkEncodeGray.
To view, visit this change. To unsubscribe, visit settings.
Cezar Espinola (Gerrit)
Cezar Espinola uploaded patch set #3 to this change.
image/png: reduce memory allocs encoding images by reusing buffers This change allows greatly reducing memory allocations with a slightly performance improvement as well. Instances of (*png).Encoder can have a optional BufferPool attached to them. This allows reusing temporary buffers used when encoding a new image. This buffers include instances to zlib.Writer and bufio.Writer. Also, buffers for current and previous rows are saved in the encoder instance and reused as long as their cap() is enough to fit the current image row. A new benchmark was added to demonstrate the performance improvement when setting a BufferPool to an Encoder instance: $ go test -bench BenchmarkEncodeGray -benchmem BenchmarkEncodeGray-4 1000 2349584 ns/op 130.75 MB/s 852230 B/op 32 allocs/op BenchmarkEncodeGrayWithBufferPool-4 1000 2241650 ns/op 137.04 MB/s 900 B/op 3 allocs/op Change-Id: I4488201ae53cb2ad010c68c1e0118ee12beae14e --- M src/image/png/writer.go M src/image/png/writer_test.go 2 files changed, 108 insertions(+), 24 deletions(-)
To view, visit this change. To unsubscribe, visit settings.
Cezar Espinola (Gerrit)
Cezar Espinola posted comments on this change.
It might be worth renaming Buffer to EncoderBuffer, in case we want a Decod
Done, I also renamed BufferPool to EncoderBufferPool due to this.
Patch Set #2, Line 49:
zwl *zwLevelI don't think you need the extra pointer here. Instead, put the two fields
Done
Patch Set #2, Line 308:
if e.zwl == nil || e.zwl.level != level {This becomes
Done
Patch Set #2, Line 499:
e.err = e.writeImage(e, e.m, e.cb, levelToZlib(e.enc.CompressionLevel))That first arg should be s/e/e.bw/
Patch Set #2, Line 224:
func BenchmarkEncodeGrayWithBufferPool(b *testing.B) {I'd put this function next to BenchmarkEncodeGray.
Done
To view, visit this change. To unsubscribe, visit settings.
Nigel Tao (Gerrit)
Nigel Tao posted comments on this change.
Patch Set 3:
(3 comments)
Just a few final nits. Fix those, and I'll submit this after Go 1.8 is released.
Thanks for looking at this.
Patch Set #3, Line 21:
// BufferPool optionally specifies a buffer pool to get temporary Bufferss/Buffers/EncoderBuffers/ or s/Buffers/buffers/
Patch Set #3, Line 27:
// instances of the Buffer struct. This can be used to reuse buffers whenDitto.
Patch Set #3, Line 34:
// EncoderBuffer is a struct used to store temporary information required whens/is a struct used to store/stores/
or simply
// EncoderBuffer holds the buffers used for encoding PNG images.
To view, visit this change. To unsubscribe, visit settings.
Cezar Espinola (Gerrit)
Cezar Espinola uploaded patch set #4 to this change.
image/png: reduce memory allocs encoding images by reusing buffers This change allows greatly reducing memory allocations with a slightly performance improvement as well. Instances of (*png).Encoder can have a optional BufferPool attached to them. This allows reusing temporary buffers used when encoding a new image. This buffers include instances to zlib.Writer and bufio.Writer. Also, buffers for current and previous rows are saved in the encoder instance and reused as long as their cap() is enough to fit the current image row. A new benchmark was added to demonstrate the performance improvement when setting a BufferPool to an Encoder instance: $ go test -bench BenchmarkEncodeGray -benchmem BenchmarkEncodeGray-4 1000 2349584 ns/op 130.75 MB/s 852230 B/op 32 allocs/op BenchmarkEncodeGrayWithBufferPool-4 1000 2241650 ns/op 137.04 MB/s 900 B/op 3 allocs/op Change-Id: I4488201ae53cb2ad010c68c1e0118ee12beae14e --- M src/image/png/writer.go M src/image/png/writer_test.go 2 files changed, 107 insertions(+), 24 deletions(-)
To view, visit this change. To unsubscribe, visit settings.
Cezar Espinola (Gerrit)
Cezar Espinola posted comments on this change.
Patch set 4:
Yay, it's great to know this is getting in. Thanks for reviewing!
(3 comments)
Patch Set #3, Line 21:
// BufferPool optionally specifies a buffer pool to get temporary
s/Buffers/EncoderBuffers/ or s/Buffers/buffers/
Done
Patch Set #3, Line 27:
// instances of the EncoderBuffer struct. This can be used to reuse buffersDitto.
Done
Patch Set #3, Line 34:
// EncoderBuffer holds the buffers used for encoding PNG images.
s/is a struct used to store/stores/
Done
To view, visit this change. To unsubscribe, visit settings.
Gobot Gobot (Gerrit)
Gobot Gobot posted comments on this change.
Patch set 4:
TryBots beginning. Status page: http://farmer.golang.org/try?commit=712cf8c3
To view, visit change 34150. To unsubscribe, visit settings.
Nigel Tao (Gerrit)
Nigel Tao posted comments on this change.
Patch set 4:Run-TryBot +1Code-Review +2
Gobot Gobot (Gerrit)
Gobot Gobot posted comments on this change.
Patch set 4:TryBot-Result +1
TryBots are happy.
Nigel Tao (Gerrit)
Nigel Tao merged this change.
image/png: reduce memory allocs encoding images by reusing buffers This change allows greatly reducing memory allocations with a slightly performance improvement as well. Instances of (*png).Encoder can have a optional BufferPool attached to them. This allows reusing temporary buffers used when encoding a new image. This buffers include instances to zlib.Writer and bufio.Writer. Also, buffers for current and previous rows are saved in the encoder instance and reused as long as their cap() is enough to fit the current image row. A new benchmark was added to demonstrate the performance improvement when setting a BufferPool to an Encoder instance: $ go test -bench BenchmarkEncodeGray -benchmem BenchmarkEncodeGray-4 1000 2349584 ns/op 130.75 MB/s 852230 B/op 32 allocs/op BenchmarkEncodeGrayWithBufferPool-4 1000 2241650 ns/op 137.04 MB/s 900 B/op 3 allocs/op Change-Id: I4488201ae53cb2ad010c68c1e0118ee12beae14e Reviewed-on: https://go-review.googlesource.com/34150 Reviewed-by: Nigel Tao <nige...@golang.org> Run-TryBot: Nigel Tao <nige...@golang.org> TryBot-Result: Gobot Gobot <go...@golang.org> --- M src/image/png/writer.go M src/image/png/writer_test.go 2 files changed, 107 insertions(+), 24 deletions(-)
diff --git a/src/image/png/writer.go b/src/image/png/writer.go
index dd87d81..49f1ad2 100644
--- a/src/image/png/writer.go
+++ b/src/image/png/writer.go
@@ -17,17 +17,37 @@
// Encoder configures encoding PNG images.
type Encoder struct {
CompressionLevel CompressionLevel
+
+ // BufferPool optionally specifies a buffer pool to get temporary
+ // EncoderBuffers when encoding an image.
+ BufferPool EncoderBufferPool
}
+// EncoderBufferPool is an interface for getting and returning temporary
+// instances of the EncoderBuffer struct. This can be used to reuse buffers
+// when encoding multiple images.
+type EncoderBufferPool interface {
+ Get() *EncoderBuffer
+ Put(*EncoderBuffer)
+}
+
+// EncoderBuffer holds the buffers used for encoding PNG images.
+type EncoderBuffer encoder
+
type encoder struct {
- enc *Encoder
- w io.Writer
- m image.Image
- cb int
- err error
- header [8]byte
- footer [4]byte
- tmp [4 * 256]byte
+ enc *Encoder
+ w io.Writer
+ m image.Image
+ cb int
+ err error
+ header [8]byte
+ footer [4]byte
+ tmp [4 * 256]byte
+ cr [nFilter][]uint8
+ pr []uint8
+ zw *zlib.Writer
+ zwLevel int
+ bw *bufio.Writer
}
type CompressionLevel int
@@ -273,12 +293,24 @@
return filter
}
-func writeImage(w io.Writer, m image.Image, cb int, level int) error {
- zw, err := zlib.NewWriterLevel(w, level)
- if err != nil {
- return err
+func zeroMemory(v []uint8) {
+ for i := range v {
+ v[i] = 0
}
- defer zw.Close()
+}
+
+func (e *encoder) writeImage(w io.Writer, m image.Image, cb int, level int) error {
+ if e.zw == nil || e.zwLevel != level {
+ zw, err := zlib.NewWriterLevel(w, level)
+ if err != nil {
+ return err
+ }
+ e.zw = zw
+ e.zwLevel = level
+ } else {
+ e.zw.Reset(w)
+ }
+ defer e.zw.Close()
bpp := 0 // Bytes per pixel.
@@ -304,12 +336,23 @@
// other PNG filter types. These buffers are allocated once and re-used for each row.
// The +1 is for the per-row filter type, which is at cr[*][0].
b := m.Bounds()
- var cr [nFilter][]uint8
- for i := range cr {
- cr[i] = make([]uint8, 1+bpp*b.Dx())
- cr[i][0] = uint8(i)
+ sz := 1 + bpp*b.Dx()
+ for i := range e.cr {
+ if cap(e.cr[i]) < sz {
+ e.cr[i] = make([]uint8, sz)
+ } else {
+ e.cr[i] = e.cr[i][:sz]
+ }
+ e.cr[i][0] = uint8(i)
}
- pr := make([]uint8, 1+bpp*b.Dx())
+ cr := e.cr
gray, _ := m.(*image.Gray)
rgba, _ := m.(*image.RGBA)
@@ -429,7 +472,7 @@
}
// Write the compressed bytes.
- if _, err := zw.Write(cr[f]); err != nil {
+ if _, err := e.zw.Write(cr[f]); err != nil {
return err
}
@@ -444,13 +487,16 @@
if e.err != nil {
return
}
- var bw *bufio.Writer
- bw = bufio.NewWriterSize(e, 1<<15)
- e.err = writeImage(bw, e.m, e.cb, levelToZlib(e.enc.CompressionLevel))
+ if e.bw == nil {
+ e.bw = bufio.NewWriterSize(e, 1<<15)
+ } else {
+ e.bw.Reset(e)
+ }
+ e.err = e.writeImage(e.bw, e.m, e.cb, levelToZlib(e.enc.CompressionLevel))
if e.err != nil {
return
}
- e.err = bw.Flush()
+ e.err = e.bw.Flush()
}
// This function is required because we want the zero value of
@@ -489,7 +535,19 @@
return FormatError("invalid image size: " + strconv.FormatInt(mw, 10) + "x" + strconv.FormatInt(mh, 10))
}
- var e encoder
+ var e *encoder
+ if enc.BufferPool != nil {
+ buffer := enc.BufferPool.Get()
+ e = (*encoder)(buffer)
+
+ }
+ if e == nil {
+ e = &encoder{}
+ }
+ if enc.BufferPool != nil {
+ defer enc.BufferPool.Put((*EncoderBuffer)(e))
+ }
+
e.enc = enc
e.w = w
e.m = m
diff --git a/src/image/png/writer_test.go b/src/image/png/writer_test.go
index d67a815..b1f97b1 100644
--- a/src/image/png/writer_test.go
+++ b/src/image/png/writer_test.go
@@ -130,6 +130,31 @@
}
}
+type pool struct {
+ b *EncoderBuffer
+}
+
+func (p *pool) Get() *EncoderBuffer {
+ return p.b
+}
+
+func (p *pool) Put(b *EncoderBuffer) {
+ p.b = b
+}
+
+func BenchmarkEncodeGrayWithBufferPool(b *testing.B) {
+ b.StopTimer()
+ img := image.NewGray(image.Rect(0, 0, 640, 480))
+ e := Encoder{
+ BufferPool: &pool{},
+ }
+ b.SetBytes(640 * 480 * 1)
+ b.StartTimer()
+ for i := 0; i < b.N; i++ {
+ e.Encode(ioutil.Discard, img)
+ }
+}
+
func BenchmarkEncodeNRGBOpaque(b *testing.B) {
b.StopTimer()
img := image.NewNRGBA(image.Rect(0, 0, 640, 480))
To view, visit change 34150. To unsubscribe, visit settings.
Nigel Tao (Gerrit)
Nigel Tao posted comments on this change.
Patch set 5:
RELNOTE=yes