573583 - enable decode-on-draw

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Bobby Holley (:bholley)]

separated out from bug 563088.

Comment 1

[Joe Drew (not getting mail)]

This blocks some beta, not necessarily beta 2.

Comment 2

[IU]

When is this going to be turned on?  I've been running with DoD for weeks and haven't noticed any issues, so I'm hoping this will make Fx 4.0, as it's a huge perf/footprint win.

Comment 3

[Bobby Holley (:bholley)]

(In reply to comment #2)
> When is this going to be turned on?  I've been running with DoD for weeks and
> haven't noticed any issues, so I'm hoping this will make Fx 4.0, as it's a huge
> perf/footprint win.

Is it really? All the image.mem.decodeondraw pref does these days (as far as I can tell) is makes us not decode images loaded in background tabs when you control+click (instead of decoding those images and then discarding them 15 seconds later). Most of the snazzy work was folded into discarding, which is already turned on by default.

Since I think it's likely that users will tab over to those pages within 15 seconds, I'm starting to think that we don't want to do this for desktop.

Can you verify the perf/footprint effects and give examples?

Thanks for your help.

Comment 4

[Dave Garrett]

(In reply to comment #3)
> Since I think it's likely that users will tab over to those pages within 15
> seconds

This is not always a good assumption. There are plenty of different browser use habits out there, and one of which is opening dozens of tabs at once and then slowly going through them one at a time.

(In reply to comment #3)
> makes us not decode images loaded in background tabs when you control+click

Which is something I and others do all the time. (and I have not enough RAM in this laptop) This will help some people in a signifigant way.

Will it hurt anyone in any significant way? This is the bit of info needed to know if it's worth turning on. If it could potentially hurt other use methods, then some sort of smart system to decide when to do what would be needed.

Comment 5

[IU]

(In reply to comment #3)
> Can you verify the perf/footprint effects and give examples?

An example I had before no longer works, as the page was long ago redesigned so that there was no longer a huge page of high-res images.  Back then, I would easily hit over 800 MB opening up the page in a background tab; the tab would remain loading for several minutes, and overall browser responsiveness would suffer.  So, I'm not sure why you don't believe this is a worthwhile benefit.  I realize you're not saying that explicitly, but that is the unfortunate result.

As for discarding, you can observe the benefit on a site like http://dekku.nofatclips.com/.  As you keep scrolling that page, the page gets longer and longer and more and more images are added.  It used to be able to increase memory consumption indefinitely (well, until you ran out).  Now memory consumption stays reasonably low.

D2D bugs aside, All I know is that I no longer come close to cracking 1 GB memory consumption since using discarding + DoD.  In fact I rarely ever hit 500 MB anymore -- something that used to happen very easily.

> Is it really? All the image.mem.decodeondraw pref does these days...

Wasn't aware of that, but performance is performance; and I'll take what I can get. ;-)
 
> Since I think it's likely that users will tab over to those pages within 15
> seconds, I'm starting to think that we don't want to do this for desktop.

I'm with Dave on this one.  If there's no *reasonable* negative to enabling an immensely beneficial feature, why forgo based on a remote possibility of an abnormal use case?  And my browsing habit, when opening tabs in the background, is similar to Dave's.  I open up a whole whack then go through them one-by-one.  If the one I am looking at has interesting information, it will definitely be a whole lot more than 15 seconds before I switch to another tab.

I can't see why a user would open up a bunch a tabs in the background and then quickly toggle through them.  Is the user more interested in making tabs flash or actually doing something even remotely productive?  And even if this is some theoretical user's habit, the user experience should be orders of magnitude better with DoD than old behavior of having the browser slow to a crawl for several minutes, while a bunch of background tabs load.

That said, hasn't Mozilla's position generally been to support features that benefit the vast majority of users and leave the special-casing to extensions?  To me, the scenario you describe seems very special-case and likely very rare.

All that being said, I do hope you will reconsider.

Many thanks

Comment 6

[Mikko Rantalainen]

I think it's not reasonable to decode not-visible images in background tabs always and then proceed to discard the work after 15 seconds. If I open 10 tabs from google results page, the changes are high that I will not see the last within 15 seconds. Using CPU to decode images on that tab is wasted CPU time.

I'd prefer not wasting memory and CPU by default even if it causes some flicker when changing to a new tab because of decode-on-draw happening at tab switch. In the long run, I'd suggest following heuristics:

* decode images on visible portion of current page (obviously) and in addition, decode images below and above the visible portion for, say, up to 10 million pixels (if the non-visible page contains a lot of small images, such as user icons in discussion forums, decoding all of them before hand should not use too much memory. On the other hand, if the page contains 20 pieces of 10 megapixel photos, it doesn't make any sense to decode all of them beforehand. In addition, counting pixels is easy because the code just needs to multiply width and height and add to total).
* decode images on "visible" (viewport) portion of non-visible tab if the tab is immediately left or right from the current tab (or perhaps keep a pixel counter here too and pre-decode visible portions of more tabs left and right from current tab if not-too-much pixels are decoded.)
* in every other case, decode the image on draw.

Especially note that if a newly opened non-visible tab is far from the current tab, the changes are high that it will not be visited within 15 seconds.

As an end result, the pre-rendering/pre-decoding takes predictable (and hopefully adjustable) amount of memory and other images may flicker a bit because of asynchronous decoding.

Comment 7

[Dave Garrett]

I've been running Firefox betas and nightlies with image.mem.decodeondraw=true for months now and have been quite pleased with the results.

I think it's worth flipping this on ASAP to get full user testing in the next betas. Any possibility of getting this flipped on for beta 8? (at least 9?)

Comment 8

[DB Cooper]

Thanks to libjpeg-turbo, performance when switching to background loaded tabs is very good now. Will decode-on-draw be enabled by default in FF 5 or 6?

Comment 9

[DB Cooper]

This is a good page for testing background loaded tab switching performance:

http://www.theatlantic.com/infocus/2011/04/texas-wildfires/100050/

Comment 10

[Joe Drew (not getting mail)]

Attachment: enable decode-on-draw

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Joe, can you land this?

Comment 12

[(no longer active)]

Landed on inbound.

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Mozilla inbound Talos shows a 3.5% Tp4 increase on XP, was it expected?

Comment 14

[Joe Drew (not getting mail)]

No - I've backed this change out. http://hg.mozilla.org/integration/mozilla-inbound/rev/e21897959746

Comment 15

[Jeff Muizelaar [:jrmuizel]]

It looks like the talos hit was on other platforms too:
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=97b2a9eeab9c&newRev=3168a26e1efa&tests=a11y,tdhtml,tdhtml_nochrome,tp4,tp4_memset,tp4_pbytes,tp4_rss,tp4_shutdown,tp4_xres,dromaeo_basics,dromaeo_css,dromaeo_dom,dromaeo_jslib,dromaeo_sunspider,dromaeo_v8,tsspider,tsspider_nochrome,tgfx,tgfx_nochrome,tscroll,tsvg,tsvg_opacity,ts,ts_cold,ts_cold_generated_max,ts_cold_generated_max_shutdown,ts_cold_generated_med,ts_cold_generated_med_shutdown,ts_cold_generated_min,ts_cold_generated_min_shutdown,ts_cold_shutdown,ts_places_generated_max,ts_places_generated_max_shutdown,ts_places_generated_med,ts_places_generated_med_shutdown,ts_places_generated_min,ts_places_generated_min_shutdown,ts_shutdown,twinopen&submit=true

mlb.com is one of the pages that shows the regression pretty clearly.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

I've added some telemetry to try to figure out what's going on here:
With decode-on-draw we decode many images twice, however, one of the decodes is just a 'size-decode' which is relatively cheap.

I counted the amount of times we paint an image and didn't notice a statistically significant difference.

One thing that was interesting was that with decode-on-draw we spend noticeably less time decoding:

65 samples, average = 48.2, sum = 3136
vs.
67 samples, average = 82.6, sum = 5537

Comment 18

[aaron]

decode-on-draw will only help when it decodes only the images in the visible part (and its neighbor screens) of the a page, or it is totally useless and will make the things worse.

For example, if we open a image heavy page which have 200 big images and will take up 3G byte when they are fully decoded.

WITHOUT decode-on-draw: firefox will take a long time to decode all images and keep them in memory. After that firefox will work well, except we loss 3G byte memory.

WITH decode-on-draw: EVERYTIME this big page actived, firefox will take a long time to decode all images. It will use 3G byte memory. This will make firefox almost unusable, if we switch to other tab and then switch back. 

THE REASON for decode-on-draw should be protecting firefox from out of memory when visiting such large pages. However if decode-on-draw decodes all images in current page, the program will still have the problem of out of memory. for example, if we switch from a 3G byte page to another 3G byte page, it will take 6G byte, and easily run out of memory.

Conclusion is, decode-on-draw should decode only part of the page in the active page, or it is just a joke.

Comment 19

[aaron]

One more comment:

decode-on-draw should be an idea of page rander, instead of garbage collector.

Comment 20

[Ahmad Saleem]

What about doing something like Page Visibility API? Page Visibility API can compensate it little bit, I think. Forgive my dumbness if feel useless reply.

Comment 21

[Bobby Holley (:bholley)]

(In reply to Ahmad Saleem from comment #20)
> What about doing something like Page Visibility API? Page Visibility API can
> compensate it little bit, I think. Forgive my dumbness if feel useless reply.

That would be useful, yes. The issue thus far is that this is rather hard to get right. But if there were other compelling uses for such an API, it might provide more motivation to implement it.

Comment 22

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #21)
> That would be useful, yes. The issue thus far is that this is rather hard to
> get right. But if there were other compelling uses for such an API, it might
> provide more motivation to implement it.

Oh, whoops - I didn't realize that Page Visibility API refers to a webkit thing. We do something like that internally (see nsIDocShell::IsActive()), but we don't expose it to content.

What we really need to mitigate the discarding issue is a callback mechanism for the visibility of particular bits of content (ie, whether they're scrolled out of view or not). But again: hard.

Comment 23

[Ahmad Saleem]

http://www.w3.org/TR/page-visibility/
I think its not limited to WebKit only. What about render only images up to desktop resolution and half to the resolution next (means half of resolution) so total 1 full resolution frame while half (0.5) next of resolution. Something like this. I know its difficult but I trust you guys. :-)

Comment 24

[Justin Lebar (not reading bugmail)]

Marking for MemShrink triage.  See e.g. bug 682230.

Comment 25

[Nicholas Nethercote [inactive]]

jrmuizel will investigate the regression some more.

Comment 26

[Jeff Muizelaar [:jrmuizel]]

My best guess is that this causes performance regressions because now we're doing size decodes in addition to regular decodes. I'll try to find some more evidence for that theory.

Comment 27

[Jeff Muizelaar [:jrmuizel]]

Looks like my theory was wrong.

The following patch seems to fix the performance regression:

diff --git a/modules/libpr0n/src/RasterImage.cpp b/modules/libpr0n/src/RasterImage.cpp
--- a/modules/libpr0n/src/RasterImage.cpp
+++ b/modules/libpr0n/src/RasterImage.cpp
@@ -2140,17 +2140,17 @@ bool

 bool
 RasterImage::StoringSourceData() {
-  return (mDecodeOnDraw || mDiscardable);
+  return ((mDecodeOnDraw && false) || mDiscardable);
 }

Comment 28

[Joe Drew (not getting mail)]

Not storing source data basically makes decode-on-draw moot for image types that aren't discardable.

Also, as far as I can tell we turn off decode-on-draw for images that aren't discardable in imgRequest::OnDataAvailable(). So I guess I'm interested in when that is the case!

Comment 30

[Jeff Muizelaar [:jrmuizel]]

For what ever reason, I can't reproduce the talos regression locally anymore...

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/519f498256da

Comment 32

[Nicholas Nethercote [inactive]]

Oh wow, this is on by default now?  That should make a huge difference to our image memory usage problems, right?

Comment 33

[Justin Lebar (not reading bugmail)]

I thought this makes a difference only when you load a new tab in the background.  With DoD we won't decode those images.  Without DoD, we'll decode them, and then throw them away 10s later.

Comment 34

[Joe Drew (not getting mail)]

What Justin says is true. We will still decode all images on a particular tab when we switch to it.

Comment 35

[Nicholas Nethercote [inactive]]

So this change only removes the decode-then-discard-after-10-seconds dance when you open a tab in the background, is that right?

Comment 36

[Joe Drew (not getting mail)]

Correct.