Closed Bug 792199 Opened 7 years ago Closed 7 years ago

Only decode visible images immediately when switching to a tab

Categories

(Core :: ImageLib, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
Tracking Status
firefox18 --- disabled

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

(Blocks 1 open bug)

Details

(Whiteboard: [snappy])

Attachments

(2 files, 1 obsolete file)

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.
Depends on: tabswitch
Whiteboard: [snappy]
Blocks: tabswitch
No longer depends on: tabswitch
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.
Attached patch Ugly patch that does the job (obsolete) — Splinter Review
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)
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+
Attached patch A nicer patchSplinter Review
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+
Depends on: 792954
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.
Also probably make the commit message a bit more descriptive.
https://hg.mozilla.org/mozilla-central/rev/68dd2bcc51ee
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(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)
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.
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.
Jeff, since we've now transitioned to Aurora, we should back this out of that branch.
Depends on: 799335
[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?
(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.
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.
(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.
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
Depends on: 802390
You need to log in before you can comment on or make changes to this bug.