1256603 - Many copies of decoded image retained at different sizes

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P3)

Description

[FateRover]

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

Comment 1

[FateRover]

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)

Comment 2

[Loic]

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.

Comment 3

[Timothy Nikkel (:tnikkel)]

I can't seem to reproduce this...the page doesn't seem to load fully.

Comment 4

[Loic]

It loads but with some time. :)
But maybe the issue is restricted to Windows.

Comment 5

[Daniel Holbert [:dholbert]]

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

Comment 6

[Daniel Holbert [:dholbert]]

[didn't finish my last sentence there, sorry]
...I think this is things working as-intended.

Comment 7

[Timothy Nikkel (:tnikkel)]

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.

Comment 8

[Daniel Holbert [:dholbert]]

Ah, didn't notice that -- thanks.

Comment 9

[Timothy Nikkel (:tnikkel)]

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.

Comment 10

[FateRover]

Attachment: TEST.7z

local archive

Comment 11

[FateRover]

  may associate with https://bugzilla.mozilla.org/show_bug.cgi?id=1123976

Comment 12

[Timothy Nikkel (:tnikkel)]

Thank you, that helps.

Comment 13

[Timothy Nikkel (:tnikkel)]

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.

Comment 14

[FateRover]

(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

Comment 15

[FateRover]

(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

Comment 16

[FateRover]

any progress？

Comment 17

[Timothy Nikkel (:tnikkel)]

(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.

Comment 18

[Timothy Nikkel (:tnikkel)]

Attachment: only mark images used if we use them

Comment 19

[Timothy Nikkel (:tnikkel)]

Attachment: make aMarkUsed required

Not sure if we should land this, but I did it anyway to make sure I had everything covered.

Comment 20

[Timothy Nikkel (:tnikkel)]

Attachment: make multipart image use RequestDecode

Comment 21

[Timothy Nikkel (:tnikkel)]

Attachment: fix imgIContainer comment

Comment 22

[Timothy Nikkel (:tnikkel)]

Attachment: fix imageloader comment

Comment 23

[Andrew Osmond [:aosmond] (he/him)]

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.)

Comment 24

[Timothy Nikkel (:tnikkel)]

(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.

Comment 25

[Andrew Osmond [:aosmond] (he/him)]

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.

Comment 26

[Timothy Nikkel (:tnikkel)]

(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.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
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

Comment 28

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/8421b16c011b
https://hg.mozilla.org/mozilla-central/rev/479653ce6d8c
https://hg.mozilla.org/mozilla-central/rev/d8bb90ade0d6
https://hg.mozilla.org/mozilla-central/rev/2cc5b3f51ec4

Comment 29

[Pulsebot]

Pushed by [e-mail]:
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

Comment 30

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/ec0b10e9a616

Comment 31

[Ionuț Goldan [:igoldan]]

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