Hi Suzy,

Can you take a look and redirect as appropriate? I quickly left it running for a few hours and memory was apparently increasing, although not dramatically.

Thanks

Heap profiling suggests that the main memory usage is in cc.debug and cc/trees, in a raster stackframe. (See pid 37218 in the attached traces.) Passing over to compositor folks for a look.

Deleted: trace_ml-start.json.gz 13.0 MB

trace_ml-start.json.gz 13.0 MB Download

Deleted: trace_ml-end.json.gz 11.0 MB

trace_ml-end.json.gz 11.0 MB Download

suzyh@ Ping! Can we get any latest update on this issue?

301 Redirect to flackr from compositing land.

Ok, took a trace with heap profiling (attached), and was able to narrow this down.

Basically, in this case, we have an SVG with a blur + merge filter. Each frame we animate the SVG and feed it through the filter. This means that each frame, we add a new entry to our filter cache, and to SkImageFilter::fCacheKeys. The cache has a memory limit, and won't grow too much, however the set of keys stored in SkImageFilter::fCacheKeys has no limit, and these keys are never removed. This means that SkImageFilter::fCacheKeys can pretty much grow unbounded.

It appears that we need to put a limit on the number of entries in skImageFilter::fCacheKeys.

bsalomon@ or senorblanco@, do you know a good person to take a look at this?

Deleted: trace_skia_leak_merged.json.gz 3.8 MB

trace_skia_leak_merged.json.gz 3.8 MB Download

Hmm. That leak was supposed to have been fixed by https://codereview.chromium.org/1651433002. Maybe something went wrong in the ID mapping.

Hey Stephen, not sure if there is anything left to do here or not but this seems to be in your domain.

Xida, could you take a look, or bounce back to me if not?

Here are some options, after discussing with Xida. I think I like option 2 the best so far.

option 1:

- add SkImageFilter* to Value
- in SkImageFilterCache::removeInternal(), call back to SkImageFilter::removeKey(key) when a value is destroyed
- pro: fairly straightforward, not too much code
- con: increases memory usage in SkImageFilterCache (one more * per Value)
- con: SkImageFilter::removeKey() would be O(N) in the number of keys in SkImageFilter::fCacheKeys

option 2:
- same as option 1, but also change fCacheKeys to a hash set
- now SkImageFilter::removeKey() can be O(1)
- pro: fairly straightforward, not too much code
- pro: SkImageFilter::~SkImageFilter() would have to iterate over the hash set (which it was doing anyway, it was just a list)
- con: increases memory usage in SkImageFilterCache (one more * per Value)

option 3:

- remove SkImageFilter::fCacheKeys altogether
- In SkImageFilter::~SkImageFilter(), call into SkImageFilterCache and have it remove all keys and values
  referring to that SkImageFilter.
  
- pro: simplifies code
- pro: reduces memory usage in SkImageFilter
- con: would be O(N) in the total number of cache entries (across all nodes)
- con: increases memory usage in SkImageFilterCache (one more * per Value)

After thinking about it some more, it may be worth trying #3 first. It's the simplest, uses no more memory than the other options (and often less) and the O(N) perf on SkImageFilter destruction is probably not too bad -- I don't think we'll be seeing millions of entries in the SkImageFilterCache (it's limited in size, after all). We can keep an eye on the bots to see if it becomes a perf bottleneck.

While I agree that option 3 is the simplest, I worry that it will cause some perf regression. Here is the story.

A while ago I had this CL: https://codereview.chromium.org/1514893003/ to fix a memory leak problem. I believe the state after this CL is exactly the same as option 3.

With the above CL landed, we had a regression here: https://bugs.chromium.org/p/chromium/issues/detail?id=571655, and our solution was this CL: https://codereview.chromium.org/1651433002/ which is to add an array to keep set of keys in the SkImageFilter.

So maybe start with option 2?

Thanks for refreshing my memory. I agree: it sounds like option 2 is the best approach.

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

commit 185a3798db64c64d47ef89a5fd3d4c5c70f1e621
Author: xidachen <xidachen@chromium.org>
Date: Thu Jun 29 15:51:17 2017

Fix memory leak in SkImageFilter

In our current implementation of SkImageFilterCache, when the
removeInternal() function is called, the Value is removed, but their
corresponding keys are not always removed in SkImageFilter. That could
result in memory leak.

In this CL, we made changes such that the Value structure now keeps
a pointer to the SkImageFilter. Each time when the removeInternal()
is called, we ask the SkImageFilter to remove the associated keys.

Bug:  689740 
Change-Id: I0807fa3581881ad1530536df5289e3976792281f
Reviewed-on: https://skia-review.googlesource.com/20960
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Mike Reed <reed@google.com>
Reviewed-by: Stephen White <senorblanco@chromium.org>
Reviewed-by: Mike Klein <mtklein@chromium.org>

[modify] https://crrev.com/185a3798db64c64d47ef89a5fd3d4c5c70f1e621/include/core/SkImageFilter.h
[modify] https://crrev.com/185a3798db64c64d47ef89a5fd3d4c5c70f1e621/tests/ImageFilterCacheTest.cpp
[modify] https://crrev.com/185a3798db64c64d47ef89a5fd3d4c5c70f1e621/src/core/SkImageFilter.cpp
[modify] https://crrev.com/185a3798db64c64d47ef89a5fd3d4c5c70f1e621/src/core/SkImageFilterCache.cpp
[modify] https://crrev.com/185a3798db64c64d47ef89a5fd3d4c5c70f1e621/src/core/SkImageFilterCache.h