Many copies of decoded image retained at different sizes

RESOLVED FIXED in Firefox 64

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
9 months ago

People

(Reporter: faterover, Assigned: tnikkel)

Tracking

({regression})

35 Branch
mozilla64
x86_64
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 wontfix, firefox62 wontfix, firefox63 wontfix, firefox64 fixed)

Details

(Whiteboard: gfx-noted [MemShrink:P2])

Attachments

(6 attachments)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20150124010859

Steps to reproduce:

1 install flash
2 enable or disable HA(try both two sets,)
3 open  http://wenku.baidu.com/view/787a850e336c1eb91a375d50.html 
see the memory usage


Actual results:

memory usage grows to 1gb


Expected results:

 memory usage of old versions isnt grown
OS: Unspecified → Windows
Hardware: Unspecified → x86_64
Component: Untriaged → Plug-ins
Product: Firefox → Core
actually can reproduce without flash;
did a memory report,most were iamages

1,288.42 MB (100.0%) -- explicit
├────892.99 MB (69.31%) -- images
│    ├──891.64 MB (69.20%) -- content
│    │  ├──891.60 MB (69.20%) -- raster
│    │  │  ├──891.60 MB (69.20%) -- used
│    │  │  │  ├──413.38 MB (32.08%) -- image(893x24199, http://bcs.wenku.bdimg.com/docconvert4787-nj/%2Fwk%2Fa517f0ab71d8e88719dc471ecf24d6b3%2F0.png?sign=MBOT:y1jXjmMD4FchJHFHIGN4z:Td1ABz5Tw3QTw8YGv3pFvg15WBk%3D&time=1458105147&range=15323-37893&response-cache-control=max-age=3888000&response-expires=Sat%2C%20 (truncated))
│    │  │  │  │  ├──413.35 MB (32.08%) -- unlocked
│    │  │  │  │  │  ├───82.44 MB (06.40%) -- surface(893x24199@0)
│    │  │  │  │  │  │   ├──82.44 MB (06.40%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───44.95 MB (03.49%) -- surface(518x22747@0)
│    │  │  │  │  │  │   ├──44.95 MB (03.49%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───39.21 MB (03.04%) -- surface(518x19843@0)
│    │  │  │  │  │  │   ├──39.21 MB (03.04%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───33.95 MB (02.64%) -- surface(518x17181@0)
│    │  │  │  │  │  │   ├──33.95 MB (02.64%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───33.95 MB (02.64%) -- surface(518x17182@0)
│    │  │  │  │  │  │   ├──33.95 MB (02.64%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───32.52 MB (02.52%) -- surface(518x16456@0)
│    │  │  │  │  │  │   ├──32.52 MB (02.52%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───32.52 MB (02.52%) -- surface(518x16455@0)
│    │  │  │  │  │  │   ├──32.52 MB (02.52%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───29.17 MB (02.26%) -- surface(518x14762@0)
│    │  │  │  │  │  │   ├──29.17 MB (02.26%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───29.17 MB (02.26%) -- surface(518x14761@0)
│    │  │  │  │  │  │   ├──29.17 MB (02.26%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───27.74 MB (02.15%) -- surface(518x14036@0)
│    │  │  │  │  │  │   ├──27.74 MB (02.15%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  └───27.73 MB (02.15%) -- surface(518x14035@0)
│    │  │  │  │  │      ├──27.73 MB (02.15%) ── decoded-nonheap
│    │  │  │  │  │      └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  └────0.02 MB (00.00%) ── source
│    │  │  │  ├──351.52 MB (27.28%) -- image(893x24829, http://bcs.wenku.bdimg.com/docconvert4787-nj/%2Fwk%2Fa517f0ab71d8e88719dc471ecf24d6b3%2F0.png?sign=MBOT:y1jXjmMD4FchJHFHIGN4z:Td1ABz5Tw3QTw8YGv3pFvg15WBk%3D&time=1458105147&range=37894-60968&response-cache-control=max-age=3888000&response-expires=Sat%2C%20 (truncated))
│    │  │  │  │  ├──351.49 MB (27.28%) -- unlocked
│    │  │  │  │  │  ├───84.58 MB (06.56%) -- surface(893x24829@0)
│    │  │  │  │  │  │   ├──84.58 MB (06.56%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───40.23 MB (03.12%) -- surface(518x20360@0)
│    │  │  │  │  │  │   ├──40.23 MB (03.12%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───40.23 MB (03.12%) -- surface(518x20359@0)
│    │  │  │  │  │  │   ├──40.23 MB (03.12%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───34.84 MB (02.70%) -- surface(518x17628@0)
│    │  │  │  │  │  │   ├──34.84 MB (02.70%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───34.84 MB (02.70%) -- surface(518x17629@0)
│    │  │  │  │  │  │   ├──34.84 MB (02.70%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───29.93 MB (02.32%) -- surface(518x15145@0)
│    │  │  │  │  │  │   ├──29.93 MB (02.32%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───29.93 MB (02.32%) -- surface(518x15146@0)
│    │  │  │  │  │  │   ├──29.93 MB (02.32%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───28.46 MB (02.21%) -- surface(518x14400@0)
│    │  │  │  │  │  │   ├──28.46 MB (02.21%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  └───28.46 MB (02.21%) -- surface(518x14401@0)
│    │  │  │  │  │      ├──28.46 MB (02.21%) ── decoded-nonheap
│    │  │  │  │  │      └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  └────0.02 MB (00.00%) ── source
│    │  │  │  ├──120.88 MB (09.38%) -- image(893x16380, http://bcs.wenku.bdimg.com/docconvert4787-nj/%2Fwk%2Fa517f0ab71d8e88719dc471ecf24d6b3%2F0.png?sign=MBOT:y1jXjmMD4FchJHFHIGN4z:Td1ABz5Tw3QTw8YGv3pFvg15WBk%3D&time=1458105147&range=0-15322&response-cache-control=max-age=3888000&response-expires=Sat%2C%2030%2 (truncated))
│    │  │  │  │  ├──120.87 MB (09.38%) -- unlocked
│    │  │  │  │  │  ├───55.80 MB (04.33%) -- surface(893x16380@0)
│    │  │  │  │  │  │   ├──55.80 MB (04.33%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───27.52 MB (02.14%) -- surface(518x13923@0)
│    │  │  │  │  │  │   ├──27.52 MB (02.14%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───18.78 MB (01.46%) -- surface(518x9501@0)
│    │  │  │  │  │  │   ├──18.78 MB (01.46%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  └───18.77 MB (01.46%) -- surface(518x9500@0)
│    │  │  │  │  │      ├──18.77 MB (01.46%) ── decoded-nonheap
│    │  │  │  │  │      └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  └────0.02 MB (00.00%) ── source
│    │  │  │  └────5.82 MB (00.45%) ++ (49 tiny)
│    │  │  └────0.00 MB (00.00%) ── unused/<non-notable images>/source
│    │  └────0.04 MB (00.00%) ── vector/used/image(0x0, https://tiles-cloudfront.cdn.mozilla.net/images/583de2b339502a7726bc05736a43b18e9c891350.2193.svg)/source
│    └────1.34 MB (00.10%) ++ (2 tiny)
├────112.92 MB (08.76%) -- js-non-window
│    ├───64.22 MB (04.98%) -- zones
│    │   ├──49.56 MB (03.85%) -- zone(0xb23800)
│    │   │  ├──33.75 MB (02.62%) ++ (435 tiny)
│    │   │  └──15.81 MB (01.23%) ── unused-gc-things
│    │   └──14.66 MB (01.14%) ++ (9 tiny)
│    ├───24.66 MB (01.91%) ++ runtime
│    └───24.04 MB (01.87%) -- gc-heap
│        ├──21.39 MB (01.66%) ── unused-arenas
│        └───2.64 MB (00.20%) ++ (2 tiny)
├─────71.32 MB (05.54%) ── heap-unclassified
├─────70.40 MB (05.46%) -- window-objects
│     ├──37.37 MB (02.90%) -- top(http://wenku.baidu.com/view/787a850e336c1eb91a375d50.html, id=67)
│     │  ├──22.13 MB (01.72%) -- active
│     │  │  ├──19.93 MB (01.55%) ++ window(http://wenku.baidu.com/view/787a850e336c1eb91a375d50.html)
│     │  │  └───2.20 MB (00.17%) ++ (8 tiny)
│     │  └──15.23 MB (01.18%) ++ js-zone(0x1a958000)
│     ├──23.23 MB (01.80%) -- top(about:memory, id=41)/active/window(about:memory)
│     │  ├──22.13 MB (01.72%) -- dom
│     │  │  ├──21.61 MB (01.68%) ── orphan-nodes
│     │  │  └───0.52 MB (00.04%) ++ (5 tiny)
│     │  └───1.10 MB (00.09%) ++ (4 tiny)
│     └───9.80 MB (00.76%) ++ (7 tiny)
├─────48.58 MB (03.77%) -- heap-overhead
│     ├──42.72 MB (03.32%) ── bin-unused
│     └───5.86 MB (00.45%) ++ (3 tiny)
├─────41.96 MB (03.26%) ++ (19 tiny)
├─────32.71 MB (02.54%) -- add-ons
│     ├──30.41 MB (02.36%) -- {d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d}/js-non-window/zones/zone(0xb23800)
│     │  ├──30.03 MB (02.33%) -- compartment([System Principal], jar:file:///D:/FireFox/45/Profiles/Profiles/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/bootstrap.js (from: resource://gre/modules/addons/XPIProvider.jsm:4515))
│     │  │  ├──29.89 MB (02.32%) -- classes
│     │  │  │  ├──20.00 MB (01.55%) ++ class(Object)
│     │  │  │  └───9.88 MB (00.77%) ++ (5 tiny)
│     │  │  └───0.14 MB (00.01%) ++ (4 tiny)
│     │  └───0.38 MB (00.03%) ++ (10 tiny)
│     └───2.30 MB (00.18%) ++ (8 tiny)
└─────17.55 MB (01.36%) -- gfx
      ├──15.09 MB (01.17%) ── heap-textures
      └───2.45 MB (00.19%) ++ (5 tiny)
Component: Plug-ins → Memory Allocator
Summary: memory leak with flash on some page → memory leak with some page
Component: Memory Allocator → ImageLib
Whiteboard: gfx-noted
It's a regression:

15:19.73 INFO: Last good revision: 5bd6e09f074e (2014-09-22)
15:19.74 INFO: First bad revision: afc933adf723 (2014-09-23)
15:19.74 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5bd6e09f074e&tocha
nge=afc933adf723

Pretty sure it regressed by Bug 1060200.
Blocks: 1060200
Keywords: regression
Version: 36 Branch → 35 Branch
I can't seem to reproduce this...the page doesn't seem to load fully.
It loads but with some time. :)
But maybe the issue is restricted to Windows.
Firstly: thanks for the bug report!

I can sort of reproduce the about:memory report in comment 1, though for me the first (most expensive) image is using 113 MB, not 413 MB, and is 893x16380 instead of 893x24199.  That might be a result of my screen resolution or some other system-dependent factor -- not sure.

Regardless, this image is huge (in terms of its dimensions), so it's not surprising that it produces a huge decoded surface.

I don't think it's fair to call this issue a "leak".  This is simply our image code using the memory that's available to it, for better performance (keeping a decoded surface around in the cache, so we don't have to re-decode it later).  I believe we do this in a way where we mark the memory as available-to-be-reclaimed-if-the-OS-needs-it or something like that, too -- I forget the details but seth could comment more.

So, I think this bug is things working as-indended; hence, resolving it as INVALID.  *If* the memory usage is still high (and this image still shows up in about:memory reports) several minutes after the tab is closed, *then* I think it'd be appropriate to consider this a "leak" & a reportable bug.  But for now, I think this is
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Resolution: --- → INVALID
[didn't finish my last sentence there, sorry]
...I think this is things working as-intended.
From the memory report in comment 1, one image has many, many surfaces. We don't want to have that many surfaces for one image unless there is a good reason.
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
Ah, didn't notice that -- thanks.
If anyone can produce a mirror or local archive of the site that reproduces the problem that would be great because I am having a very hard time reproducing. The site is not very reliable. Trying to download the same image by itself in a separate tab will work once, and then time out on many attempts over an hour.
Posted file TEST.7z
local archive
Thank you, that helps.
The page is hard to debug, but it looks like there is one large (800 x 16000) image that is used as a CSS sprite map. I'm guessing the image is designed for retina screens, and then dynamically the elements using it are scaled down. The sizes are the sprites are quite small (around 20 pixels I think), so I'm guessing pulling a 20 pixel rect from a 16000 image can introduce small differences in the overall requested frame size of the image (I saw 9500, 9501, 9992).

As for what to do about this, maybe we should cap the total memory use by one image.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to Timothy Nikkel (:tnikkel) from comment #13)
> The page is hard to debug, but it looks like there is one large (800 x
> 16000) image that is used as a CSS sprite map. I'm guessing the image is
> designed for retina screens, and then dynamically the elements using it are
> scaled down. The sizes are the sprites are quite small (around 20 pixels I
> think), so I'm guessing pulling a 20 pixel rect from a 16000 image can
> introduce small differences in the overall requested frame size of the image
> (I saw 9500, 9501, 9992).
> 
> As for what to do about this, maybe we should cap the total memory use by
> one image.

actually the test file isnt load fully,there still another 11 pages to load by click ,i can only save thefirst page,you can scroll the page to simulate the last 11
(In reply to Loic from comment #2)
> It's a regression:
> 
> 15:19.73 INFO: Last good revision: 5bd6e09f074e (2014-09-22)
> 15:19.74 INFO: First bad revision: afc933adf723 (2014-09-23)
> 15:19.74 INFO: Pushlog:
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=5bd6e09f074e&tocha
> nge=afc933adf723
> 
> Pretty sure it regressed by Bug 1060200.

the link https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5bd6e09f074e&tocha  make ff & IE stunk,lol
any progress?
Summary: memory leak with some page → memory leak with some page
Whiteboard: gfx-noted → gfx-noted [MemShrink]
(In reply to Timothy Nikkel (:tnikkel) from comment #13)
> As for what to do about this, maybe we should cap the total memory use by
> one image.

A better idea would be to consider both the size of the decoded frames for the image and the size difference between the closest size already decoded and the new size being asked for. Something simple like "if the decoded image size is a few MBs and the requested size differs by less than 10% from an existing size just use the existing size". This adds some complexity as we'd be returning the WRONG_SIZE DrawResult and hence we'd be changing the meaning of WRONG_SIZE (it currently means that calling again and asking for syncdecode would result in a better frame, which would be false with this proposed change), or we'd need to detect this case and change the DrawResult to SUCCESS.
Summary: memory leak with some page → Many copies of decoded image retained at different sizes
Whiteboard: gfx-noted [MemShrink] → gfx-noted [MemShrink:P2]
Assignee: nobody → tnikkel
Not sure if we should land this, but I did it anyway to make sure I had everything covered.
Attachment #9008666 - Flags: review?(aosmond)
Attachment #9008667 - Flags: review?(aosmond)
Attachment #9008668 - Flags: review?(aosmond)
Attachment #9008669 - Flags: review?(aosmond)
Attachment #9008666 - Flags: review?(aosmond) → review+
Attachment #9008668 - Flags: review?(aosmond) → review+
Attachment #9008669 - Flags: review?(aosmond) → review+
Attachment #9008667 - Flags: review?(aosmond) → review+
Comment on attachment 9008665 [details] [diff] [review]
only mark images used if we use them

Review of attachment 9008665 [details] [diff] [review]:
-----------------------------------------------------------------

I realize I wasn't added to review this part, but I have a question for FrameAnimator::RequestRefresh:

::: image/FrameAnimator.cpp
@@ +427,5 @@
>      SurfaceCache::Lookup(ImageKey(mImage),
>                           RasterSurfaceKey(mSize,
>                                            DefaultSurfaceFlags(),
> +                                          PlaybackType::eAnimated),
> +                         /* aMarkUsed = */ false);

Is the assumption here that if the image is displayed, it will be locked, and thus cannot be evicted? RequestRefresh is otherwise the only thing that we know will happen that will mark it as used (because it could keep getting a composited frame, and be a very long animation so it takes a long time to loop back to the start.)
(In reply to Andrew Osmond [:aosmond] from comment #23)
> Comment on attachment 9008665 [details] [diff] [review]
> only mark images used if we use them
> 
> Review of attachment 9008665 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I realize I wasn't added to review this part, but I have a question for
> FrameAnimator::RequestRefresh:

That was a mistake, I meant to ask review.

> ::: image/FrameAnimator.cpp
> @@ +427,5 @@
> >      SurfaceCache::Lookup(ImageKey(mImage),
> >                           RasterSurfaceKey(mSize,
> >                                            DefaultSurfaceFlags(),
> > +                                          PlaybackType::eAnimated),
> > +                         /* aMarkUsed = */ false);
> 
> Is the assumption here that if the image is displayed, it will be locked,
> and thus cannot be evicted? RequestRefresh is otherwise the only thing that
> we know will happen that will mark it as used (because it could keep getting
> a composited frame, and be a very long animation so it takes a long time to
> loop back to the start.)

Ah, good catch. I was just setting aMarkUsed to false if the caller didn't actually use the frame data. But I guess AdvanceFrame (if it gets called after this) would use the frame data.

On the other hand, if we are calling RequestRefresh on an image that isn't displayed (if it was displayed it should be locked) then we would want to let the surface expire and get discarded.

I don't feel strongly about either way.
Attachment #9008665 - Flags: review?(aosmond)
Comment on attachment 9008665 [details] [diff] [review]
only mark images used if we use them

Review of attachment 9008665 [details] [diff] [review]:
-----------------------------------------------------------------

r+; only concern was what I commented in RequestRefresh, which I think it should mark it as used unless we are certain it will be locked in all cases.
Attachment #9008665 - Flags: review?(aosmond) → review+
(In reply to Andrew Osmond [:aosmond] from comment #25)
> Comment on attachment 9008665 [details] [diff] [review]
> only mark images used if we use them
> 
> Review of attachment 9008665 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+; only concern was what I commented in RequestRefresh, which I think it
> should mark it as used unless we are certain it will be locked in all cases.

I just changed it to mark used. We can revisit it later if it ends up mattering.
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8421b16c011b
Only mark images as used in the surface cache if we actually use them. r=aosmond
https://hg.mozilla.org/integration/mozilla-inbound/rev/479653ce6d8c
Make aMarkUsed parameters required everywhere. r=aosmond
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8bb90ade0d6
Fix comment for imgIContainer::RequestDecodeForSize to match reality. r=aosmond
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cc5b3f51ec4
Fix comment in ImageLoader. r=aosmond
Keywords: leave-open
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec0b10e9a616
Change MultipartImage to use RequestDecode instead of GetFrame to avoid marking the frame as used. r=aosmond
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/ec0b10e9a616
Status: NEW → RESOLVED
Closed: 3 years ago9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Noticed some big perf improvements:

== Change summary for alert #16781 (as of Sat, 13 Oct 2018 02:31:39 GMT) ==

Improvements:

  6%  Images windows7-32 opt stylo                             5,773,777.05 -> 5,420,867.44
  6%  Images linux64 opt stylo                                 5,919,683.15 -> 5,559,192.11
  6%  Images linux64-stylo-sequential opt stylo-sequential     5,872,879.65 -> 5,528,974.58
  4%  Images windows10-64 opt stylo                            7,029,619.06 -> 6,733,006.45
  4%  Images windows10-64-qr opt stylo                         6,897,424.11 -> 6,651,303.82

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=16781
You need to log in before you can comment on or make changes to this bug.