Only decode visible images immediately when switching to a tab

RESOLVED FIXED in mozilla18

Status

()

Core
ImageLib
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

(Blocks: 1 bug)

unspecified
mozilla18
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox18 disabled)

Details

(Whiteboard: [snappy])

Attachments

(2 attachments, 1 obsolete attachment)

18.06 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
18.03 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 789573
Whiteboard: [snappy]
(Assignee)

Updated

5 years ago
Blocks: 789573
No longer depends on: 789573
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
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?
Attachment #664939 - Flags: feedback?(khuey)
Attachment #664939 - Flags: feedback?(justin.lebar+bug)
(Assignee)

Updated

5 years ago
Attachment #664939 - Flags: feedback?(joe)
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/
Attachment #664939 - Flags: feedback?(justin.lebar+bug) → feedback+
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.
Attachment #664939 - Flags: feedback?(joe) → feedback+
(Assignee)

Comment 5

5 years ago
Created attachment 666636 [details] [diff] [review]
A nicer patch
Attachment #664939 - Attachment is obsolete: true
Attachment #664939 - Flags: feedback?(khuey)
Attachment #666636 - Flags: review?(joe)
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
Attachment #666636 - Flags: review?(joe) → review+
(Assignee)

Updated

5 years ago
Depends on: 792954
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/68dd2bcc51ee
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).
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

5 years ago
Also probably make the commit message a bit more descriptive.
https://hg.mozilla.org/mozilla-central/rev/68dd2bcc51ee
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(Assignee)

Comment 12

5 years ago
(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.
startDecoding(); in the .idl files needs some documentation.

How risky is this? This seems to have helped a lot with tab switching.
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

5 years ago
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
(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

5 years ago
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.
We can't identify whether images are visible.
See Bug 689623.
Jeff, since we've now transitioned to Aurora, we should back this out of that branch.
Depends on: 799335
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
Attachment #669640 - Flags: approval-mozilla-aurora?

Comment 22

5 years ago
(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

5 years ago
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).
Yes, it's easy to be fast if you're wrong, e.g. bug 799335.
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.
(Assignee)

Comment 26

5 years ago
(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.
Attachment #669640 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
https://hg.mozilla.org/releases/mozilla-aurora/rev/d405ef60655d
status-firefox18: --- → disabled
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!
It's possible, depending on what that bug was caused by :)
Depends on: 804000
(Assignee)

Updated

5 years ago
Depends on: 802390
You need to log in before you can comment on or make changes to this bug.