All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=758946

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=4a0470b6d291290b76f21b6f3b24c073bea9e82ca3f0dbf57965fe6ccb7bb6bc


Bot(s) for this bug's original alert(s):

chromium-rel-win10

Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8970287731708866736

=== Auto-CCing suspected CL author cblume@chromium.org ===

Hi cblume@chromium.org, the bisect results pointed to your CL, please take a look at the
results.


=== BISECT JOB RESULTS ===
Perf regression found with culprit

Suspected Commit
  Author : cblume
  Commit : 4fed3346549a90c0de40c02f6388e19e8151e92a
  Date   : Thu Aug 17 18:19:18 2017
  Subject: Use SkCodec internally in GIFImageDecoder

Bisect Details
  Configuration: winx64_10_perf_bisect
  Benchmark    : system_health.memory_desktop
  Metric       : memory:chrome:all_processes:reported_by_chrome:malloc:effective_size_avg/browse_media/browse_media_tumblr
  Change       : 1.83% | 75377832.6429 -> 76758788.1111

Revision             Result                   N
chromium@495175      75377833 +- 2919604      14      good
chromium@495224      75098736 +- 3878741      14      good
chromium@495228      75319695 +- 2215415      9       good
chromium@495229      74695240 +- 3509110      9       good
chromium@495230      76891360 +- 4523614      14      bad       <--
chromium@495231      77119684 +- 1868079      9       bad
chromium@495237      77181841 +- 2027297      6       bad
chromium@495249      77363103 +- 1249084      6       bad
chromium@495273      76758788 +- 2685493      9       bad

Please refer to the following doc on diagnosing memory regressions:
  https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_benchmarks.md

To Run This Test
  src/tools/perf/run_benchmark -v --browser=release_x64 --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=browse.media.tumblr system_health.memory_desktop

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8970287731708866736


For feedback, file a bug with component Speed>Bisection

This may be expected. But it may not.

scroggo@ - Do we expect SkGifCodec to be using more memory internally? I would imagine not.

vmpstr@ - Does CC maintain a previous frame in its cache which is no longer needed? Details below.

tl;dr -- I think this is because ImageBitmap::GetSkImageFromDecoder isn't purging the old frames, and the average frame size is now larger.


Previously, we would only store the part of an image frame which was updated. So for example, if we had a 300x300 animated image the first frame might contain data for all 300x300 pixels. But the second frame might update only a small portion of that (say, 50x50 pixels). Previously, the image decoder would store the complete first frame and the updated region of the second frame.

But with the move to SkCodec, what we provide to / receive from the SkCodec API are complete frames. That would mean the second frame (which still gets stored) would be 300x300 instead of 50x50.

That said, the previous frame is no longer needed with this API change. It should be getting purged immediately after we attempt to decode the second frame.

- Maybe this memory report is happening after decode but before purge. I suspect that isn't what is happening.
- Maybe the purge isn't called somewhere? I think this is more likely. Although ImageFrameGenerator seems to always purge, ImageBitmap::GetSkImageFromDecoder doesn't seem to purge. So maybe the increased second frame size is visible because of this?

Also, although ImageFrameGenerator purges the ImageDecoder cache fairly aggressively, I'm unsure about CC's cache and how often it is purged. If it isn't purged as aggressively, the larger frame sizes may be the cause.

> scroggo@ - Do we expect SkGifCodec to be using more memory internally?
> I would imagine not.

No. SkGifCodec is designed to work with non-buffered sources, though, in which case it will store the LZW blocks. It is possible we accidentally made SegmentStream look like a non-buffered source, but looking over the code, that does not appear to be the case.

> tl;dr -- I think this is because ImageBitmap::GetSkImageFromDecoder isn't
> purging the old frames, and the average frame size is now larger.

I'm confused; that method appears to just decode frame 0 and then delete the ImageDecoder. So even if the decoder had old frames, they would be deleted by that method, right? And it appears to only be called with new ImageDecoders anyway.

> Previously, we would only store the part of an image frame which was
> updated. So for example, if we had a 300x300 animated image the first
> frame might contain data for all 300x300 pixels. But the second frame
> might update only a small portion of that (say, 50x50 pixels).
> Previously, the image decoder would store the complete first frame
> and the updated region of the second frame.

Where is this code? You're saying that ImageDecoder only stores the updated region, or some other class? ImageDecoder stores ImageFrames, each of which is the size of the full image (e.g. 300 x 300 in your example), regardless of the updated portion. (See ImageFrame::AllocatePixelData, ::TakeBitmapDataIfWritable, and ::CopyBitmapData.)

> But with the move to SkCodec, what we provide to / receive from the
> SkCodec API are complete frames. That would mean the second frame
> (which still gets stored) would be 300x300 instead of 50x50.

You mean because SkCodec does not properly set the OriginalFrameRect? That only appears to be accessed internally to ImageDecoder/ImageFrame, and I don't see how it results in storing less memory.

> I'm confused; that method appears to just decode frame 0 and then delete the ImageDecoder. So even if the decoder had old frames, they would be deleted by that method, right? And it appears to only be called with new ImageDecoders anyway.

When it decodes frame 0, that causes ImageDecoder to check how many frames there are (and initialize the new frames).

Since SkCodec parses as far as it can (rather than parsing as needed), it might be aware of frames beyond frame 0. And so when we decode frame 0, we might also allocate space for frame 1.

For ImageFrameGenerator, this isn't a big deal because we'll be using those frames. But for ImageBitmap::GetSkImageFromDecoder it could potentially be extra allocated memory that wasn't allocated before.

The memory usage increased by 2.2 MiB. That seems large enough that it might be image frame data, which is why I'm looking there.


> ImageDecoder stores ImageFrames, each of which is the size of the full image (e.g. 300 x 300 in your example), regardless of the updated portion. (See ImageFrame::AllocatePixelData, ::TakeBitmapDataIfWritable, and ::CopyBitmapData.)

You're right. I thought we might use the frame rect for allocation size. But the functions you mentioned clearly don't use it. The entire image size is allocated each frame. So that also wouldn't be a source of the increased memory usage.

Oh, I meant to add:
You are right that the ImageDecoder is cleaned up immediately after frame 0 is decoded in ImageBitmap::GetSkImageFromDecoder. So even if an extra frame 1 is allocated, it will be deallocated shortly after.

So the likelihood of this being the actual cause of the reported memory increase is small.

Okay....I'm open to other ideas? :D

> Since SkCodec parses as far as it can (rather than parsing as needed),
> it might be aware of frames beyond frame 0. And so when we decode
> frame 0, we might also allocate space for frame 1.

This hasn't actually changed. Notice how ImageBitmap::GetSkImageFromDecoder calls FrameCount. This method parses the whole file, both before and after switching to SkCodec. But we don't allocate the pixel memory until it's time to decode. (InitializeNewFrame just sets some bookkeeping values on ImageFrame. InitFrameBuffer allocates the pixel memory.)

> Okay....I'm open to other ideas? :D

SkGifCodec uses an std::vector internally, whereas the old code used a WTF::Vector. Due to potentially different growth factors, we may be using more memory.

SkGifCodec cannot use WTF::Vector (Skia cannot depend on Blink), but could use SkTArray, which I believe shares a 1.5x growth factor with WTF::Vector.

The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/e750391c349ff1fa1c179fbf2f5af27fb4405cef

commit e750391c349ff1fa1c179fbf2f5af27fb4405cef
Author: Leon Scroggins III <scroggo@google.com>
Date: Wed Aug 30 15:06:54 2017

GIF: Use SkTArray instead of std::vector

Speculative fix for a memory regression seen in Chromium. Chromium
previously used a WTF::Vector, which has a growth factor of 1.5, as does
SkTArray. Depending on the implementation of std::vector, this may slow
the allocation of memory.

Bug:  758946 
Change-Id: I323390027467e32a6c66667c927fae0aba292446
Reviewed-on: https://skia-review.googlesource.com/40777
Reviewed-by: Ben Wagner <bungeman@google.com>
Commit-Queue: Leon Scroggins <scroggo@google.com>

[modify] https://crrev.com/e750391c349ff1fa1c179fbf2f5af27fb4405cef/third_party/gif/SkGifImageReader.cpp
[modify] https://crrev.com/e750391c349ff1fa1c179fbf2f5af27fb4405cef/third_party/gif/SkGifImageReader.h

I was hoping to look at the graph again now that the SkTArray change landed, but the linked graph stops before the fix. Is there a way to extend it further forward in time? Or do we just not have the data yet, past 8/28? Searching manually for the same test seems to stop at the same point.

I see the graph updated through today, including a Skia roll.
The last build tested included:
https://chromium.googlesource.com/chromium/src/+log/ffcf38006384c0c377b0cbcf29e7b317aceaf879%5E..5aca1690c0ade593dbaa61e1165f02949da1408b?pretty=fuller&n=

Below the graph is a zoomed-out view with a green overlay which can be dragged around. You can use that to get to the furthest-right (most recent).

The latest measurement is better than what it seemed to be stable at with the 3.1% regression. So this seems to have helped. It isn't yet clear if that completely fixed the regression though. A few more data points might start to show a trend that is in the pre-regression zone. But at the moment, it seems to still be a very slight regression.

The graph is a bit noisy, but it looks like this is fixed as per the discussion in #13.

You might be right.
The ref is a tad noisy, which is a sign that something might be up...bots getting warm or something. So I don't know that this is conclusively fixed. I'll keep an eye on it.

Chiming in to say it does indeed seem fixed. I said I would keep an eye on it and it seems to be stable at the same place as _ref.