Last Comment Bug 792199 - Only decode visible images immediately when switching to a tab
: Only decode visible images immediately when switching to a tab
Status: RESOLVED FIXED
[snappy]
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal with 2 votes (vote)
: mozilla18
Assigned To: Jeff Muizelaar [:jrmuizel]
:
Mentors:
Depends on: 792954 799335 802390 804000
Blocks: tabswitch
  Show dependency treegraph
 
Reported: 2012-09-18 13:11 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2012-12-03 14:29 PST (History)
26 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
disabled


Attachments
Ugly patch that does the job (9.99 KB, patch)
2012-09-26 07:02 PDT, Jeff Muizelaar [:jrmuizel]
justin.lebar+bug: feedback+
joe: feedback+
Details | Diff | Review
A nicer patch (18.06 KB, patch)
2012-10-01 11:47 PDT, Jeff Muizelaar [:jrmuizel]
joe: review+
Details | Diff | Review
back out of aurora (18.03 KB, patch)
2012-10-09 11:20 PDT, Joe Drew (not getting mail)
gavin.sharp: approval‑mozilla‑aurora+
Details | Diff | Review

Description Jeff Muizelaar [:jrmuizel] 2012-09-18 13:11:30 PDT
We currently decodeabit of all of the images on a page when we make it active. Instead we should only decodeabitof when we draw the images, and leave the rest of the images for the worker.
Comment 1 Jeff Muizelaar [:jrmuizel] 2012-09-18 13:13:48 PDT
We should be able to do this by making RequestDecode take a parameter that specifies whether we should DecodeABitOf. We can then use that in EnumerateLock.
Comment 2 Jeff Muizelaar [:jrmuizel] 2012-09-26 07:02:44 PDT
Created attachment 664939 [details] [diff] [review]
Ugly patch that does the job

Passing around bools everywhere is ugly but the lack of enumerated types in xpcom makes the alternative bad too.

At this point I'm more interested in feedback on the behaviour change. Does this seem sane to everyone?
Comment 3 Justin Lebar (not reading bugmail) 2012-09-26 08:15:14 PDT
Comment on attachment 664939 [details] [diff] [review]
Ugly patch that does the job

The bool-ness would be much nicer IMO if you did

 RequestDecode(/* decodeSomeImmediately */ true)

As an alternative, having two methods seems even better to me.

  RequestDecodeWithSomeSync()
  RequestDecodeWithNoSync()

You can collapse them into one method with a bool arg in RasterImage.

> +  NS_IMETHOD RequestDecode(bool decodeSome = true);

If we believe explicit is better than implicit [1], we should get rid of the default arg.

I guess I'd have to play around with the patch to judge whether it's an improvement.  We synchronously decode a bit on draw, so we don't get a white rectangle for /every/ image on tab switch?

FYI, If you use git bz to attach your patches, it should automatically do -U8.

[1] http://www.python.org/dev/peps/pep-0020/
Comment 4 Joe Drew (not getting mail) 2012-09-26 11:37:36 PDT
Comment on attachment 664939 [details] [diff] [review]
Ugly patch that does the job

I am supportive of this. We should put it in at the beginning of a cycle, though, so we shake out possible issues.
Comment 5 Jeff Muizelaar [:jrmuizel] 2012-10-01 11:47:43 PDT
Created attachment 666636 [details] [diff] [review]
A nicer patch
Comment 6 Joe Drew (not getting mail) 2012-10-02 09:25:45 PDT
Comment on attachment 666636 [details] [diff] [review]
A nicer patch

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

I think that you forgot the part where we call RequestDecode() when switching tabs, but other than that and some nits below, looks wonderful.

::: image/public/imgIContainer.idl
@@ +231,5 @@
>     * decoded but don't want to access it until then, you must call
>     * requestDecode().
>     */
>    void requestDecode();
> +  void startDecoding();

change uuid and document the difference between these two

::: image/public/imgIRequest.idl
@@ +145,5 @@
>     * container already exists, or calls it once it gets OnStartContainer if the
>     * container does not yet exist.
>     */
>    void requestDecode();
> +  void startDecoding();

change uuid and document the difference between these two

::: image/src/RasterImage.cpp
@@ +2412,5 @@
> +
> +NS_IMETHODIMP
> +RasterImage::RequestDecode()
> +{
> +    return RequestDecodeCore(false);

indentation is wrong

@@ +2418,5 @@
> +
> +NS_IMETHODIMP
> +RasterImage::StartDecoding()
> +{
> +    return RequestDecodeCore(true);

indentation is wrong

::: image/src/RasterImage.h
@@ +575,5 @@
>                              uint32_t **paletteData, uint32_t *paletteLength);
>  
>    bool ApplyDecodeFlags(uint32_t aNewFlags);
>  
> +  NS_IMETHOD RequestDecodeCore(bool decodeSome);

change this boolean to an enumerated type

::: image/src/VectorImage.cpp
@@ +574,5 @@
>    // Nothing to do for SVG images
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP

add 
/* void startDecoding() */

::: image/src/imgRequest.cpp
@@ +505,5 @@
> +    return mImage->StartDecoding();
> +  }
> +
> +  // Otherwise, flag to do it when we get the image
> +  mDecodeRequested = true;

This isn't an "otherwise."

::: image/src/imgRequestProxy.cpp
@@ +329,5 @@
>    // Forward the request
>    return mOwner->RequestDecode();
>  }
>  
> +

remove whitespace
Comment 7 Jeff Muizelaar [:jrmuizel] 2012-10-04 13:09:16 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/68dd2bcc51ee
Comment 8 Joe Drew (not getting mail) 2012-10-04 17:29:52 PDT
You didn't do this:


::: image/public/imgIRequest.idl
@@ +145,5 @@
>     * container already exists, or calls it once it gets OnStartContainer if the
>     * container does not yet exist.
>     */
>    void requestDecode();
> +  void startDecoding();

change uuid and document the difference between these two

Also:

+  NS_IMETHOD RequestDecodeCore(RequestDecodeType aDecodeType);

This should just have an nsresult return type, since it's not part of the interface ("I" method).
Comment 9 Joe Drew (not getting mail) 2012-10-04 17:31:06 PDT
OH. And I said this:

> I am supportive of this. We should put it in at the beginning of a cycle, though, so we shake out 
> possible issues.

So we should probably back this out.
Comment 10 :aceman 2012-10-05 02:08:39 PDT
Also probably make the commit message a bit more descriptive.
Comment 11 Ed Morley [:emorley] 2012-10-05 04:01:33 PDT
https://hg.mozilla.org/mozilla-central/rev/68dd2bcc51ee
Comment 12 Jeff Muizelaar [:jrmuizel] 2012-10-05 06:03:12 PDT
(In reply to Joe Drew (:JOEDREW! \o/) from comment #9)
> OH. And I said this:
> 
> > I am supportive of this. We should put it in at the beginning of a cycle, though, so we shake out 
> > possible issues.
> 
> So we should probably back this out.

I was planing on backing it out of Aurora when the switch happens instead of postponing landing so that we get more testing.
Comment 13 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-10-06 07:05:12 PDT
startDecoding(); in the .idl files needs some documentation.

How risky is this? This seems to have helped a lot with tab switching.
Comment 14 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-10-06 07:11:15 PDT
I guess this is risky, since some images don't get decoded now. (I'll file a bug if I can reproduce
the problem)
Comment 15 dindog 2012-10-08 07:42:17 PDT
Do I suppose to see some improvement when opening page with tons of images? 
I don't notice any, a page used to took 2GB RAM, which 99% of the photos is *not* visible, still took 2GB RAM in today's nightly
Comment 16 Justin Lebar (not reading bugmail) 2012-10-08 07:44:22 PDT
(In reply to dindog from comment #15)
> Do I suppose to see some improvement when opening page with tons of images? 
> I don't notice any, a page used to took 2GB RAM, which 99% of the photos is
> *not* visible, still took 2GB RAM in today's nightly

That's not what this bug fixes, confusingly enough.  You're looking for bug 689623.
Comment 17 Jake 2012-10-08 16:20:01 PDT
How about don't discard visible images when switching to other tabs? since we can already identify whether images are visible, it will improve tab switching even more, only cost a little more memory.
Adding "image.mem.discard_visible" to "about:config", people can choose at will.
Comment 18 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-10-08 16:34:03 PDT
We can't identify whether images are visible.
Comment 19 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-10-08 16:34:37 PDT
See Bug 689623.
Comment 20 Joe Drew (not getting mail) 2012-10-09 07:51:58 PDT
Jeff, since we've now transitioned to Aurora, we should back this out of that branch.
Comment 21 Joe Drew (not getting mail) 2012-10-09 11:20:03 PDT
Created attachment 669640 [details] [diff] [review]
back out of aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): this bug
User impact if declined: Unfinished work, needs to be reverted so we can finish
Testing completed (on m-c, etc.): revert to pre-existing situation
Risk to taking this patch (and alternatives if risky): little bit jankier tab switch, i.e., what we have had all the time
String or UUID changes made by this patch: Changes imgIContainer and imgIRequest back to what they were prior to this patch
Comment 22 (dormant account) 2012-10-09 15:43:19 PDT
(In reply to Joe Drew (:JOEDREW! \o/) from comment #21)
> Created attachment 669640 [details] [diff] [review]
> back out of aurora
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): this bug
> User impact if declined: Unfinished work, needs to be reverted so we can
> finish
> Testing completed (on m-c, etc.): revert to pre-existing situation
> Risk to taking this patch (and alternatives if risky): little bit jankier
> tab switch, i.e., what we have had all the time
> String or UUID changes made by this patch: Changes imgIContainer and
> imgIRequest back to what they were prior to this patch

By a little jankier, you mean 30-40% slower tab switch times as per http://is.gd/fPVXJY. This was a pretty awesome change.
Comment 23 (dormant account) 2012-10-09 15:58:10 PDT
s/30-40%/50%/, see saved-session data in http://is.gd/AI1Ysw(saved-session pings have more data than idle-daily since they capture multiple sessions per day).
Comment 24 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-10-09 16:04:44 PDT
Yes, it's easy to be fast if you're wrong, e.g. bug 799335.
Comment 25 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-10-09 16:17:32 PDT
Well, bug 799335 isn't necessarily wrong. I personally favor the current way faster tab switching 
over the never-paint-wrong-when-switch-to-a-tab behavior.
Comment 26 Jeff Muizelaar [:jrmuizel] 2012-10-09 16:35:27 PDT
(In reply to Olli Pettay [:smaug] from comment #25)
> Well, bug 799335 isn't necessarily wrong. I personally favor the current way
> faster tab switching 
> over the never-paint-wrong-when-switch-to-a-tab behavior.

Hopefully, with the correct version of this patch we'll be able to get a good compromise between both of them.
Comment 27 Joe Drew (not getting mail) 2012-10-10 07:24:39 PDT
(In reply to Taras Glek (:taras) from comment #23)
> By a little jankier, you mean 30-40% slower tab switch times as per
> http://is.gd/fPVXJY. This was a pretty awesome change.

Don't worry, this isn't going away long-term, only for 18 (it should be in 19). As I said in comment 4, though, this needs more testing and fixing before going in long-term; it's utterly broken right now, despite some people's preference for that behaviour. I can virtually guarantee that bug 799335 isn't the only thing that will need fixing.
Comment 28 Joe Drew (not getting mail) 2012-10-10 07:27:32 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/d405ef60655d
Comment 29 David Tenser [:djst] 2012-10-15 13:22:44 PDT
Is this possibly the fix for bug 694859? Especially the original STR in that bug suggests the slowness is pronounced on image heavy pages. Regardless, yay perf!
Comment 30 Joe Drew (not getting mail) 2012-10-15 13:28:44 PDT
It's possible, depending on what that bug was caused by :)

Note You need to log in before you can comment on or make changes to this bug.