Loaded the repro case and grabbed a memory-infra trace, which shows the memory is allocated by cc. Dana, could you take a look?

wez, if you still have the trace, could you upload it?

The first memory-infra trace I took right after loading the page. The second I took after idling for ~20 seconds. Notice cc memory consumption goes from ~50MB -> ~400MB. 

Deleted: trace_memory_gif1.json.gz 1.8 MB

trace_memory_gif1.json.gz 1.8 MB Download

Deleted: trace_memory_gif2.json.gz 2.2 MB

trace_memory_gif2.json.gz 2.2 MB Download

From the trace it seems that each frame of that gif is being cached in our software image cache (the sizes match up: 720x405 image is ~1140k). It also seems that none of these segments are locked, since we would report them as locked in the memory dump.

It could be the difference in discardable memory implementation of the two different systems, or maybe we're reporting something different on two different systems.

Is it possible to get a trace on a "good" machine for comparsion?

Also, have you seen this memory go beyond 512mb? IIRC if the discardable system is not the true discardable, then it has a 512mb limit and then it starts evicting unlocked things. If that's the case we're hitting, then this might be expected. In other words, it might be 512mb across all processes, it just happens that this tab is using most of it.

I'm going to try and repro on my mac and on linux to see if I can get the comparisons.

Re #5: I've just repro'd on ChromeOS, loading the page in my main profile and in incognito, and confirmed that loading it in two separate renderers does indeed cause the two to max-out at a lower usage, i.e. the 500MB does seem to be the browser-wide limit.

+haraken

This seems like a place where we should be trying to make an intelligent tradeoff between memory and performance. Perhaps a good place for memory coordinator to make a difference.

e.g. maybe we should be using a large cache when there's plenty of free memory, and a smaller cache when there's not very much free memory.

ericrk@: I was assuming that CC is already listening to MemoryPressureListener (currently) and MemoryCoordinator (in the future).

Does this bug mean that the pressure signal is not working as expected? Or it's working as expected but the memory usage bumps up just because there's a lot of available memory in the system (and thus the pressure signal is not sent)?

FWIW I am trivially able to repro this CC bloat w/ the linked site on Mac
(12GB RAM) and ChromeOS (4GB RAM), but not on Windows (64GB RAM), so it
doesn't seem related to available memory.

Re #7/#8, the memory here is owned by CC, but is all unlocked discardable, which CC treats as ~free. We will purge this memory if the coordinator sends us a purge command, but I believe this only happens in the background.

CC doesn't have any real limits on how much unlocked discardable it holds - it assumes that unlocked memory will be freed out from under it if memory pressure arises.

It does seem unfortunate that we treat this memory as ~free, but it still shows up counted against Chrome in various task managers.

Currently it seems like discardable memory has an upper limit of 512MB on desktop and 128MB on Android. On a page with sufficient images, we'll basically hit this limit and stay there until the page is closed.

Discardable memory already has a mechanism to purge the unlocked memory when the memory pressure signal is sent. So I think that the problem is that SystemMonitor is not thinking that the memory pressure is high and thus the pressure signal is not dispatched.

+chrisha +shrike

re #11, erikchen@ and wez@, correct me if I'm wrong, but I don't think this was a case where we necessarily expected the memory pressure signal to be sent, we just noticed that Chrome seemed to be using more memory than it should be? So not sure anything is behaving wrong in the pressure system.

Not sure what we want to do here - it seems like more of a messaging issue than an actual issue. Chrome is using a fair amount of unlocked discardable memory (512mb) to store objects for speculative re-use, but we have plenty of memory, so we aren't purging this due to pressure. Unfortunately, this means that Chrome appears to be using additional MBs, even though this memory could be reclaimed by the system at any time. We could tweak numbers in our task manager, but then it wouldn't line up with system task managers.

We might be able to do something a bit smarter in the case of animated gifs - The gif in question is 487 frames, each frame being 1.1mb, which means it will never fit in our 512mb discardable cache, and no cached items will ever be re-used. It also means that the gif will evict everything else in discardable as it plays. We may want to exempt gifs over a certain number of frames from being cached in discardable. vmpstr@, is this something that might make sense, or is it tricky to implement?

mlamouri, jaikk, dalecurtis: Have there been thoughts on how to better handle repeating animated images? For looping videos, I assume we just decode in each loop. Why do we treat images differently?

Re: c#11, we don't necessarily want to purge memory on a memory pressure signal because that can exacerbate the memory pressure situation. If discardable memory is implemented as pages of VM that can just be let go of, that would probably be OK to do in response to a memory pressure signal. If discardable memory is chunks of malloced memory, that could be bad (e.g. if the machine is swapping, having to page in chunks of memory just to free() will make swapping worse).

I believe discardable memory is not based on VM pages, in which case we would want to discard it based on a signal that memory resources are getting tight (which is a goal of the Memory Coordinator/MemoryManager).

Yes, the plan of Memory Coordinator is to send the signals before the system starts compressing or swapping.

cc:hbengali

Re: c#13, holding into a compressed version of the animated image in memory rather than a decoded version does sound reasonable. Its possible very small animated images can be handled differently based on the memory / latency (of decode) trade off. 

> but not on Windows (64GB RAM)

Why are we not using the same discardable memory amounts on this windows machine?

> We could tweak numbers in our task manager, but then it wouldn't line up with system task managers.

We could show separate used memory vs discardable memory, that would add up to the system numbers.

Re #17:

> Why are we not using the same discardable memory amounts on this windows machine?

I took a trace from the Windows machine, and it appears that we are using the same amount of memory as reported by the discardable system. It seems like the Chrome and Windows task managers do not count this memory against us, maybe due to differences in how discardable works on Windows. Attached the trace.

> We could show separate used memory vs discardable memory, that would add up to the system numbers.

This seems like a good idea.

Deleted: trace_windows_gif.json.gz 818 KB

trace_windows_gif.json.gz 818 KB Download

Re #13 & #16

The caching being discussed here isn't related to the gif decoder. This cache is managed by CC, and is used for all images (regardless of type). CC doesn't actually know the images it's dealing with are GIFs, instead the image looks like any other image, so CC caches it normally. Blink then updates the image every frame, and CC re-renders / re-caches.

It sounds like it's not a great idea for CC to cache much of an animated gif (especially a long one) - my guess is we'd need to pass additional info from blink > CC to identify when an image like this shouldn't be cached.

@#12, this is certainly something that we should implement. However, at this stage in the pipeline we don't currently know if the images we're decoding are a part of an animated sequence (and we don't know how many more frames are expected before we loop). With CDL work, we should be able to plumb this information. I think if we determine that the animated image can't fit in memory in its totality, we can go as aggressive as  caching only two frames of the image (the last frame and the current frame). When we get a new frame, we would make current frame be the last frame and reuse the last frame's memory for the new frame.

I'll file a separate bug for this. 

Regarding memory-pressure, I've just tried re-loading that GIF on my Chromebox and saw it balloon to 500+MB again - as previously noted that system has only 4GB of physical memory, and memory usage was ~3.6/4GB physical and ~3.6/4GB zram swap in-use at the time, with the system already janking from kswapd0 activity.

Deleted: Screenshot 2017-02-22 at 10.30.13 - Display 2.png 36.6 KB

	Screenshot 2017-02-22 at 10.30.13 - Display 2.png 36.6 KB View Download

filed crbug.com/695108 for GIF improvements in the CC cache.

As for this bug, I get a sense that what we get here is expected behavior, but the reporting might be better. Is that the case? If so, I propose we file specific bugs to improve discardable memory reporting and close this bug.

wez@ haraken@ does this sound reasonable?

Hmm, or maybe we should do something smarter with the discardable system (sorry, didn't see comment #21 before posting #22)

Re #23: Yes, it seems that something is broken in the pressure->discard path - may be as simple as our thresholds being inappropriate.

We used to have code in blink that limited to cache size for animated GIFs to 5MB. If we went above that then only the last frame would be cached. That worked well for animated GIFs as decoding the next frame is typically cheap when you have the previous frame. 

However, this was pre- CC image cache management. We might want to introduce a similar mechanism in CC. 

+1 to c#12. Letting the whole image decode in memory is a bit crazy, we don't even allow our compressed cache to accumulate that much memory :)

FWIW, this file is 2050 frames at 720x405 resolution with 4 bytes per pixel or ~2280mb of data if fully decoded into RGBA. So there must be some cap already imposed in the cache; it's just a bit too high.

Your cache should probably be some fixed size or specifically based on frame count. Probably a generic system needs both to support high resolution use cases. I.e. for media we only keep 4-8 frames available at any given time.

Err, +1 to c#25, c#12 is cool too, but not what I intended :)

Yeah I think we are in agreement about making cc cache smarter wrt gifs. I filed a bug in #22 for that. FWIW the cc cache is for all images that need rasterization. We don't currently bundle frames coming from the same GIF as being related in any way. This needs to change.

The locked portion of this cache only stays locked for duration of raster. Other than that, it stays in cache in a unlocked state in case we can reuse it later. As ericrk@ mentioned, we essentially treat unlocked discardable memory as free. This bug is certainly the case where we're unintentionally thrashing the cache.

I can't really think of a good solution that would work in a general case, so I think this still depends on us plumbing more information from blink to cc.

Specifically, I don't think we should go the route where we have a second layer of caching. If we need a chunk of B bytes, I would still prefer to use discardable memory in general for that and rely on it to recycle any other B sized buffers as needed. The alternative, would be that we try to reuse B sized buffers we've previously allocated and that haven't been used/locked in a while but are still lockable from within the cc cache.

However, in the specific case of GIFs, once we know which images are coming from the same GIF, then we can certainly do something better without too much complexity. (Something like restrict the buffers/amount of memory used for one GIF as opposed to one frame of a GIF). 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7d1142a765ae5648b212d357e4db425e2fad3623

commit 7d1142a765ae5648b212d357e4db425e2fad3623
Author: vmpstr <vmpstr@chromium.org>
Date: Wed Feb 22 23:18:19 2017

cc: Always report locked_size in software image cache.

This patch ensures that we always report the locked_size, and we
report 0 if the decode is unlocked.

R=ericrk@chromium.org
BUG=693968
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2713483002
Cr-Commit-Position: refs/heads/master@{#452262}

[modify] https://crrev.com/7d1142a765ae5648b212d357e4db425e2fad3623/cc/tiles/gpu_image_decode_cache.cc
[modify] https://crrev.com/7d1142a765ae5648b212d357e4db425e2fad3623/cc/tiles/software_image_decode_cache.cc

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

=== BISECT JOB RESULTS ===
NO Perf regression found

Bisect Details
  Configuration: android_one_perf_bisect
  Benchmark    : smoothness.key_silk_cases
  Metric       : mean_frame_time/font_wipe.html

Revision             Result                  N
chromium@450360      51.4254 +- 14.9625      21      good
chromium@450438      51.7078 +- 18.121       21      bad

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=font.wipe.html smoothness.key_silk_cases

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8986826703215348672

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=4965200844816384


| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Speed>Bisection.  Thank you!

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

=== BISECT JOB RESULTS ===
NO Perf regression found

Bisect Details
  Configuration: android_one_perf_bisect
  Benchmark    : smoothness.key_silk_cases
  Metric       : mean_frame_time/font_wipe.html

Revision             Result                 N
chromium@450360      51.3741 +- 13.635      21      good
chromium@450438      51.618 +- 28.0041      21      bad

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=font.wipe.html smoothness.key_silk_cases

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8982667484729917680

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=4965200844816384


| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Speed>Bisection.  Thank you!

With stable ids for paint images landing soon, I think we can be more aggressive in cleaning up memory for images that are unlikely to be seen again (or seen soon, such as recent gif frames). 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/05729e73de37e9f74e80e1009350422142b7aae5

commit 05729e73de37e9f74e80e1009350422142b7aae5
Author: vmpstr <vmpstr@chromium.org>
Date: Tue Jun 06 03:07:33 2017

Add frame count to paint image.

This patch adds the number of frames in the image to PaintImage.

R=chrishtr@chromium.org
BUG=693968
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2883593002
Cr-Commit-Position: refs/heads/master@{#477168}

[modify] https://crrev.com/05729e73de37e9f74e80e1009350422142b7aae5/cc/paint/paint_image.cc
[modify] https://crrev.com/05729e73de37e9f74e80e1009350422142b7aae5/cc/paint/paint_image.h
[modify] https://crrev.com/05729e73de37e9f74e80e1009350422142b7aae5/third_party/WebKit/Source/platform/DragImage.cpp
[modify] https://crrev.com/05729e73de37e9f74e80e1009350422142b7aae5/third_party/WebKit/Source/platform/graphics/BitmapImage.h
[modify] https://crrev.com/05729e73de37e9f74e80e1009350422142b7aae5/third_party/WebKit/Source/platform/graphics/Image.cpp
[modify] https://crrev.com/05729e73de37e9f74e80e1009350422142b7aae5/third_party/WebKit/Source/platform/graphics/Image.h

Khushal, would you mind taking over this?

Sure, I can take this. We are now in a good spot to identify when the cached decodes are a part of an animating gif so this is much easier to implement. Re-iterating the consensus as I understood it, we should have a cap for the amount of memory allocated for all frames of a GIF. If the complete GIF can't fit within that limit, we only keep the last n frames.

Re #38: Is there any point caching frames at all, if we can't cache them all? If we cache only the last N frames then every new frame will be a cache miss, so we'll decode every frame and the cache is just wasted memory, surely?

How many frames are normally cached? We only keep 4 for video.

Re #39, you're right, it doesn't make sense to cache last n frames if we can't cache all of them since we'll be getting all cache misses as the gif loops around anyway. But we should still cache last 2 frames, for tiles on pending and active tree since different tiles on these trees will be reusing the same decode.

Re #40, we cache everything in unlocked discardable. Right now there is no distinction between frames of a gif or static images in the image cache's caching strategy.