403660 - SetFullZoom does unnecessary reflow

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Boris's comment #6 from bug 402555:

Top-down, we seem to have one obvious inefficiency: 
DocumentViewerImpl::SetFullZoom calls nsPresContext::SetFullZoom, which calls
nsViewManager::DoSetWindowDimensions, which does a resize reflow on the
presshell.  That takes up about 25% of the profile.  Then immediately after
that we call ClearStyleDataAndReflow(), which blows away all our style,
reresolves it, then generally reflows the world.  50% of the profile is this
second reflow.

Comment 1

[Daniel Brooks [:db48x]]

As a lark, I removed the second reflow. The results are interesting; text no longer changes size (though it does change position), and images only change size when you increase the zoom. I didn't have time to look any deeper though. 

Comment 2

[Boris Zbarsky [:bzbarsky]]

The right reflow to remove would be the first one, not the second. We do need to blow away the style data and do a reflow after that.  Doing that should subsume the things that the resize reflow does, I would think...

Comment 3

[Daniel Brooks [:db48x]]

Commenting out the resize reflow causes most of the UI to disappear. Maximizing the window makes it come back, but the the <browser> has zero height, and it doesn't take up the full width of the window. Other than that, it seems ok. :) My naive analysis was that fixing the resize reflow would be the better way to go.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Commenting out the resize reflow where, exactly?

We should do all of the resize reflow except the actual DoReflow() call in ResizeReflow...

Comment 5

[Daniel Brooks [:db48x]]

(In reply to comment #4)
> Commenting out the resize reflow where, exactly?

nsViewManager.h:279

> We should do all of the resize reflow except the actual DoReflow() call in
> ResizeReflow...

So the only thing from that function that needs to happen in this case is the call to SetVisibleArea? Everything else looks like it's just prepping for the call to DoReflow.

Comment 6

[Boris Zbarsky [:bzbarsky]]

That and the resize event timer creation, probably.

But again, only in the case when we're about to follow up with a full "everything is dirty" reflow as here.

Comment 7

[Daniel Brooks [:db48x]]

Perhaps if it needs to recompute all of the style data, that should happen when it calls ProcessPendingRestyles?

  // Make sure style is up to date
  mFrameConstructor->ProcessPendingRestyles();

Comment 8

[Boris Zbarsky [:bzbarsky]]

No point.  We're about to do a top-down style data recomputation anyway in ClearStyleDataAndReflow.

Comment 9

[Daniel Brooks [:db48x]]

Yea, but shouldn't there be a way to avoid that?

Comment 10

[Daniel Brooks [:db48x]]

What I mean is, even if it means that ResizeReflow occasionally does all the work that ClearStyleAndReflow would have done, it seems cleaner than having to 'manually' start the layout over again when you change the size.

Comment 11

[Boris Zbarsky [:bzbarsky]]

I'm not sure what you mean...  What exactly are you proposing should get optimized differently?

Comment 12

[Daniel Brooks [:db48x]]

Attachment: 403660-1.diff

This works pretty well. Maybe someone could profile it, I gotta get some sleep.

Comment 13

[Oleg Romashin (:romaxa)]

I have tested this patch on x86 slow pc and on device... and it did not bring any visible improvements for me....

Comment 14

[venu madhav]

Is the bug fixed?

Comment 15

[Ilpo Ruotsalainen]

Attachment: Benchmark results with and without patch

Some benchmarking results with and without the patch. Benchmarked on a 400MHz ARMv6, by adding timing to the JS that changes the zoom level in our XUL UI. 

The improvement is pretty noticeable even without looking at the timing results.

Comment 16

[venu madhav]

>>DocumentViewerImpl::SetFullZoom calls nsPresContext::SetFullZoom, which calls
>>nsViewManager::DoSetWindowDimensions, which does a resize reflow on the
>>presshell.  That takes up about 25% of the profile.  Then immediately after
>>that we call ClearStyleDataAndReflow(), which blows away all our style,
>>reresolves it, then generally reflows the world.  50% of the profile is this
>>second reflow.
In Second reflow: ClearStyleDataAndReflow() calls ComputeStyleChangeFor() and it in turn calls  ReResolveStyleContext().  ReResolveStyleContext() is actually taking more than 60% of whole time to render CSS/data recomputation. I think optimizing the operations in this function will improve performance of zooming a lot. 

Comment 17

[Boris Zbarsky [:bzbarsky]]

That has nothing to do with this bug.  If you think there are other issues that block zoom performance, please file new bugs blocking bug 402555.  Note, however, that ReResolveStyleContext is recomputing style for everything in the tree here, and style computation is already quite heavily optimized.  Making that any faster would be very hard.

Comment 18

[Daniel Brooks [:db48x]]

(In reply to comment #15)
> Created an attachment (id=297355) [details]
> Benchmark results with and without patch
> 
> Some benchmarking results with and without the patch. Benchmarked on a 400MHz
> ARMv6, by adding timing to the JS that changes the zoom level in our XUL UI. 
> 
> The improvement is pretty noticeable even without looking at the timing
> results.
> 

This is excellent Ilpo, thanks. I'll just find a reviewer next…

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 288622 [details] [diff] [review]
403660-1.diff

>Index: layout/base/nsPresContext.cpp
>+  if (mFullZoom != aZoom)
>+  {
>+    mZooming = PR_TRUE;

Can you assert that mZooming == PR_FALSE before that assignment?  Otherwise you'll clobber a true value to false when you set it below.

>Index: layout/base/nsPresContext.h
>+public:
>+  PRBool                mZooming;  

Why is this public?  We don't want the world _writing_ this!  It should be protected like the other members, with a const public getter for the one place that needs to check the value.

And it should probably be a bit in the unsigned at the end of this class (before the ibmbidi section), rather than a PRBool.

>Index: layout/base/nsPresShell.cpp
>+  if (!GetPresContext()->mZooming)

Please use the getter as described above.

The rest looks great.  r=bzbarsky with the nits fixed.

Comment 20

[Daniel Brooks [:db48x]]

(In reply to comment #19)
> (From update of attachment 288622 [details] [diff] [review])
> >Index: layout/base/nsPresContext.h
> >+public:
> >+  PRBool                mZooming;  
> 
> Why is this public?  We don't want the world _writing_ this!  It should be
> protected like the other members, with a const public getter for the one place
> that needs to check the value.

Ah, I'd forgotten about this. I've actually got this change in my tree already.

> And it should probably be a bit in the unsigned at the end of this class
> (before the ibmbidi section), rather than a PRBool.

Sure.

Comment 21

[Daniel Brooks [:db48x]]

Attachment: 403660-2.diff

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 297771 [details] [diff] [review]
403660-2.diff

>Index: layout/base/nsPresContext.cpp
>+  : mType(aType), mDocument(aDocument), mTextZoom(1.0), mFullZoom(1.0), mZooming(PR_FALSE),

This is going to cause warnings because you're initializing things in an order different from the order they're declared in.  Fix that, please?

>Index: layout/base/nsPresContext.h
>+
>+protected:

Why is this needed?

>+public:
>+  const PRBool          IsZooming() { return mZooming; }

Please put this up with other methods instead of sticking it in with the variables?  And the right signature should be more like:

  PRBool IsZooming() const { return mZooming; }

no?

r=bzbarsky with that fixed.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  if (mFullZoom != aZoom)
+  {

Don't check this, we already checked it above

Instead of mZooming I think it's more maintainable to have a flag "mSuppressResizeReflow".

Comment 24

[Daniel Brooks [:db48x]]

Attachment: 403660-4.diff

Comment 25

[Damon Sicore (:damons)]

Comment on attachment 299626 [details] [diff] [review]
403660-4.diff

This bug is a blocker, so no approval request is required.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Checked the patch in.  Daniel, thanks for doing this!

Comment 27

[Daniel Brooks [:db48x]]

Ah, I missed the blocker flag. Thanks for checking this in bz.