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.

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.

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

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.

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

See Bug 689623.

Jeff, since we've now transitioned to Aurora, we should back this out of that branch.

[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

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

(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 :)