Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix memory leak in gif thumbnail encoding #565

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix memory leak in gif thumbnail encoding #565

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 14 additions & 20 deletions thumbnailing/i/gif.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package i

import (
"bytes"
"errors"
"fmt"
"image"
"image/draw"
"image/gif"
Expand Down Expand Up @@ -76,19 +78,15 @@ func (d gifGenerator) GenerateThumbnail(b io.Reader, contentType string, width i
}

// The thumbnailer decided that it shouldn't thumbnail, so encode it ourselves
pr, pw := io.Pipe()
go func(pw *io.PipeWriter, p *image.Paletted) {
err = u.Encode(ctx, pw, p)
if err != nil {
_ = pw.CloseWithError(errors.New("gif: error encoding still frame thumbnail: " + err.Error()))
} else {
_ = pw.Close()
}
}(pw, targetImg)
buf := bytes.NewBuffer(make([]byte, 0, 128*1024))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these allocations are deliberately avoided to reduce memory volatility on large instances. This is the reason for the complicated io.Pipe stuff - if we can get that to work instead, would be best.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would require conditionally draining the pipe, which would probably introduce a fair amount of complexity, I will give it a look though. I would argue that even 50 MB of temporary allocation is preferable over a 10 MB memory leak though. This stuff adds up quickly. After running this, the normal ram usage of MMR is about 8-10 MB

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The average supported MMR deployment uses around 1gb of memory already, and is generally handling quite a few requests for thumbnails. 50mb per frame (or 50mb per thumbnail when animation is disabled) is quite a lot when considering traffic levels and performance impacts of the garbage collector.

MMR used to have "sawtooth" memory usage because of temporary buffers like this, and it was a major operational problem for hosting providers. To fix it, all code dropped in-memory buffers in favour of direct pipes or file system caching. This not only stabilized memory usage, but increased performance and throughput as well, though the underlying libraries for things like files still use a partial buffer themselves. This leads to a tiny bit of sawtooth-shaped graphs, but not nearly as bad as it could be with manual buffers.

err = u.Encode(ctx, buf, targetImg)
if err != nil {
return nil, fmt.Errorf("gif: error encoding static thumbnail: %w", err)
}
return &m.Thumbnail{
Animated: false,
ContentType: "image/png",
Reader: pr,
Reader: io.NopCloser(buf),
}, nil
}

Expand All @@ -110,20 +108,16 @@ func (d gifGenerator) GenerateThumbnail(b io.Reader, contentType string, width i
g.Config.Width = g.Image[0].Bounds().Max.X
g.Config.Height = g.Image[0].Bounds().Max.Y

pr, pw := io.Pipe()
go func(pw *io.PipeWriter, g *gif.GIF) {
err = gif.EncodeAll(pw, g)
if err != nil {
_ = pw.CloseWithError(errors.New("gif: error encoding final thumbnail: " + err.Error()))
} else {
_ = pw.Close()
}
}(pw, g)
buf := bytes.NewBuffer(make([]byte, 0, 512*1024))
err = gif.EncodeAll(buf, g)
if err != nil {
return nil, fmt.Errorf("gif: error encoding animated thumbnail: %w", err)
}

return &m.Thumbnail{
ContentType: "image/gif",
Animated: true,
Reader: pr,
Reader: io.NopCloser(buf),
}, nil
}

Expand Down