69534 - Unnecessary Reflows for animated GIF images: eliminate them now.

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Marc Attinasi]

nsImageFrame::UpdateImage is called whenever a new frame is to be displayed for
an animated GIF. If the ImageFrame has a fixed width and height, there is no
need to reflow it. Also, even if there are no width and height attributes, we
can check if the iamge size is unchanged and avoid reflowing in that case as well.

This should be a nice performance improvement, since most top100 pages have
animated GIFs and we waste a whole lot of time relowing them when they almost
never actually change size.

Comment 1

[Marc Attinasi]

Accepting my own bug - marking perf kwd.

Comment 2

[Mats Palmgren (inactive)]

Could this be causing bug 68228? If so, its not just a perf problem.

Comment 3

[Marc Attinasi]

Mats - I'm not sure if this is the same problem, though it looks like it could
be. Can you post a reduced testcase for the problem image in bug 68228? I'll cc
myself on that bug and see if it is fixed too.

Comment 4

[Kevin McCluskey (gone)]

I did some performance analysis using a release build on Linux loading cnn.com.
As a test I commented out the code which generated the reflow in
nsImageFrame::UpdateImage. The page rendered correctly without this (Probably
because the table ended up reflowing the image as content as appended.)

cnn.com rendered 10% faster.

A small change to nsImageFrame.cpp to cache the old width and height and only
reflow if it has changed should speed up page loads on pages with animated gifs.

Comment 5

[Marc Attinasi]

OK, I have a patch that checks to see if an image is fixed size (has explicit
width and height) and bypasses the reflows on notifications if so. A further
refinement is to handle the case where there is no width and height, but we know
that the image has not changed size - I'm working on that.

Comment 6

[Marc Attinasi]

Attachment: Patch to suppress reflows on animated image notifications if image is fixed width and height

Comment 7

[Brendan Eich [:brendan]]

Hyatt's playing around with similar hacks just now.

/be

Comment 8

[David Hyatt]

I just did this coincidentally.  On jrgm's tests I got about a 4% speedup overall.

Comment 9

[David Hyatt]

I did something a little different though.  You may get better results.

Comment 10

[David Hyatt]

I added code to the beginning of nsImageFrame that asked if you were doing a
resizereflow.  If you were, and your rect's width and height were the same as
the reflow state's computed width and height, I just fill in those values and
return immediately without calling GetDesiredSize, etc.  Avoids thrashing around
asking the image loader questions.

Comment 11

[Marc Attinasi]

Kevin ran my patch against John's tests and we saw a ~20% improvement in the
loadtimes - more than I expected.

This is good, so I'm gonna go a little further and handle images that do not
have an explicit width and height but we know that the intrinsic size has not
changed. I think David's change will help too, but for a different reason:
whereas my change prevents reflows on animated images, his changes will prevent
reflows that DO get through from doing any unnecessary interactions with the
imageloader.

Comment 12

[Jesse Ruderman]

Are there any cases when moving to the next frame *does* affect the size of an 
image?

Comment 13

[Marc Attinasi]

Yup, if the <img> has no width or height and the image's next frame is a 
different size, then we have to reflow because the size will change.

Comment 14

[David Hyatt]

I produced a patch coincidentally on the same stuff.  I sent it to you in email,
attinasi and commented on mozilla.performance.  I had some questions about your
patch that I sent to you in email and to mozilla.performance.

Comment 15

[David Hyatt]

sr=hyatt, although it seems that you can also fix the size for percentage
widths/heights as well, and I have questions about values of 0 when used with
width/height.

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

I'd love to see this checked in.  However, it can be extended as well and pick
up quite a few more pages/reflows.  This turns off reflows for defined
widths/heights (width=N, CSS equivalent, etc).  (Hyatt's comment about
percentages is good too, though you might need to make sure that we do reflow if
the result of the percentage changes).

Jesse Ruderman wrote:
>Are there any cases when moving to the next frame *does* affect the size of an
>image?

Marc Attinasi wrote:
>Yup, if the <img> has no width or height and the image's next frame is a
>different size, then we have to reflow because the size will change.

So, once we have a size for a single frame of an anim, if the next frame _is_
the same size, we should not reflow, right?  I don't think the patch covers that
case.

I'd be happy to see the current patch checked in; but I'd like to see these
other issues (especially the last one) dealt with as well.

Comment 17

[selmer (gone)]

For image frames that _do_ change size, would it make sense to make the
allocated size monotonically increasing so that eventually it stops reflowing?
Seems like this would only negatively impact cases where it was used to
intentionally whack the contents around for some DHTML-like effect (and that
wouldn't be done frequently would it?)

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Isn't different framesizes _very_ rare? Shouldn't we in those rare cases do the 
RightThing and resize+reflow accordingly? I think it would give a very "buggy" 
look if we never decrease imagesize.

Comment 19

[Adam D. Moss]

An animated GIF (the vast, vast majority of animated <IMG>
content) cannot change its frame ('screen') size dynamically
at all.  I don't know about other animation formats such as
MNG (I expect not, however).

Comment 20

[Marc Attinasi]

Adam, is it part of the GIF specification that the individual frames are all the 
same size? I'm not familiar with the details there, but for some reason I 
assumed that each frame could be a different size (maybe I just infered it from 
the code.) Obviously it is an easier problem if the individual images in the 
animation MUST be the same size; in that case we never have to reflow for 
subsequent frames, and I do not have to cache the previous image size either.

In either case, I'm gonna check in the simple change that checks for a fixed 
size (expanding the notion to include percentage width/height as well) and then 
worry about the case of the image size changing. Pavlov told me that his new 
image lib stuff makes this all pretty much obsolete anyway...

Comment 21

[Adam D. Moss]

Hiya.  It's part of the GIF spec that the dimensions
of the outer frame ('screen size') are specified in the
header and fixed for the duration of the GIF.  Individual
frames that constitute the animation are not necessarily
the same size but are strictly restricted to the area defined
by the outer frame[1], which should not expand and contract.  So
in short, the size of the image is fixed as far as layout is
concerned.

[1] Some broken GIFs have frames which overlap the edges of the
image bounds but that's purely imagelib's problem (it should clip
them, but AFAIK currently crashes) and not yours.

Comment 22

[Marc Attinasi]

Thanks for the data, Adam. That makes the optimization much easier.

I wonder if theis is now internalized in the new imagelib... We don't need new
dimension notifications for each frame of the animation (in GIF anyweay)

Comment 23

[Marc Attinasi]

Milestone shift --> Moz 0.8.1

Comment 24

[Marc Attinasi]

Fix checked in: nsImageFrame.h/cpp

Comment 25

[Chris Petersen]

Marking verified per last comments.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 60880 has been marked as a duplicate of this bug. ***