Closed Bug 403660 Opened 12 years ago Closed 12 years ago

SetFullZoom does unnecessary reflow

Categories

(Core :: Layout, defect, P3)

x86
All
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: vlad, Assigned: db48x)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(2 files, 2 obsolete files)

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.
Flags: blocking1.9+
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. 
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...
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.
Commenting out the resize reflow where, exactly?

We should do all of the resize reflow except the actual DoReflow() call in ResizeReflow...
(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.

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.
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();
No point.  We're about to do a top-down style data recomputation anyway in ClearStyleDataAndReflow.
Yea, but shouldn't there be a way to avoid that?
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.
I'm not sure what you mean...  What exactly are you proposing should get optimized differently?
Attached patch 403660-1.diff (obsolete) — Splinter Review
This works pretty well. Maybe someone could profile it, I gotta get some sleep.
Assignee: nobody → db48x
Status: NEW → ASSIGNED
I have tested this patch on x86 slow pc and on device... and it did not bring any visible improvements for me....

Is the bug fixed?
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.
>>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. 
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.
(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…

Attachment #288622 - Flags: review?(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.
Attachment #288622 - Flags: review?(bzbarsky) → review+
(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.
Attached patch 403660-2.diff (obsolete) — Splinter Review
Attachment #297771 - Flags: superreview?
Attachment #297771 - Flags: review?(bzbarsky)
Attachment #297771 - Flags: superreview? → superreview?(roc)
Attachment #288622 - Attachment is obsolete: true
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.
Attachment #297771 - Flags: review?(bzbarsky) → review+
+  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".
Keywords: perf
Attached patch 403660-4.diffSplinter Review
Attachment #299626 - Flags: superreview?(roc)
Attachment #297771 - Attachment is obsolete: true
Attachment #297771 - Flags: superreview?(roc)
Attachment #299626 - Flags: superreview?(roc) → superreview+
Attachment #299626 - Flags: approval1.9?
Comment on attachment 299626 [details] [diff] [review]
403660-4.diff

This bug is a blocker, so no approval request is required.
Attachment #299626 - Flags: approval1.9? → approval1.9+
Checked the patch in.  Daniel, thanks for doing this!
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Ah, I missed the blocker flag. Thanks for checking this in bz.
You need to log in before you can comment on or make changes to this bug.