913205 - New computation of FrameMetrics::mCompositionBounds causes performance regression

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Botond Ballo [:botond]]

This is a follow-up to bug 904533, to fix the performance regression noted at https://bugzilla.mozilla.org/show_bug.cgi?id=904533#c43.

tn suggested in https://bugzilla.mozilla.org/show_bug.cgi?id=904533#c44 that the cause of the regression is the added call to GetScreenRectInAppUnits which usually ends up making a system call to determine the x/y offset to return.

As it happens, the x/y offset returned also isn't really what we want - we want the offset relative to the reference frame's origin, not relative to the screen.

Comment 1

[Botond Ballo [:botond]]

Attachment: Compute offset of FrameMetrics::mCompositionBounds correctly

This patch corrects the computation of the offset of FrameMetrics::mCompositionBounds in RecordFrameMetrics(). The new computation no longer involves calling GetScreenRectInAppUnits(). It should also be more correct.

Try job for talos on linux: https://tbpl.mozilla.org/?tree=Try&rev=de78feee9449

Comment 2

[Timothy Nikkel (:tnikkel)]

Comment on attachment 800410 [details] [diff] [review]
Compute offset of FrameMetrics::mCompositionBounds correctly

Can we factor out computing scrollFrameBounds and mCompositionBounds and do it before the if, then inside the if we just have to adjust for scrollbars? The code is the same except for the frame we use. Can do it in a followup.

>@@ -3276,17 +3282,19 @@ nsDisplayScrollLayer::BuildLayer(nsDispl
>-  RecordFrameMetrics(mScrolledFrame, mScrollFrame, layer, mVisibleRect, viewport,
>+  RecordFrameMetrics(mScrolledFrame, mScrollFrame,
>+                     aBuilder->FindReferenceFrameFor(mScrolledFrame), layer,
>+                     mVisibleRect, viewport,

Display items have ReferenceFrame() that returns the saved reference frame, no need to find it again.

Comment 3

[Botond Ballo [:botond]]

Attachment: Compute offset of FrameMetrics::mCompositionBounds correctly

Updated patch to address tn's review comments. Carrying r+.

Comment 4

[Botond Ballo [:botond]]

Initial talos results are looking good.

Full try job: https://tbpl.mozilla.org/?tree=Try&rev=407963c1063f

Comment 5

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c4d78e3f9ec1

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c4d78e3f9ec1

Comment 7

[Avi Halachmi (:avih)]

(In reply to Botond Ballo [:botond] from comment #0)
> This is a follow-up to bug 904533, to fix the performance regression noted
> at https://bugzilla.mozilla.org/show_bug.cgi?id=904533#c43.

Here's a graph which shows svg-ASAP on all windows and ubuntu platforms with relevant dates range:
http://graphs.mozilla.org/graph.html#tests=[[281,131,35],[281,131,31],[281,131,25],[281,131,37],[281,131,33]]&sel=1377912724174,1378517524174&displayrange=7&datatype=running

win7/8 were unaffected, ubuntu 32/64 regressed and also got noisier, and win xp improved. Hopefully ubuntu will improve without affecting xp?

Comment 8

[Kartikaya Gupta (email:[e-mail])]

That looks to be the case. I presume the win xp improvements were caused by something else then and just happened to line up with this by coincidence.

http://graphs.mozilla.org/graph.html#tests=[[281,131,35],[281,131,31],[281,131,25],[281,131,37],[281,131,33]]&sel=1378295346554.2002,1378537621852.6643,161.2060240165672,795.5102033685672&displayrange=30&datatype=running

Comment 9

[Matt Brubeck (:mbrubeck)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> That looks to be the case. I presume the win xp improvements were caused by
> something else then and just happened to line up with this by coincidence.

Yes, the Windows XP improvements were caused by bug 907544.

Comment 10

[Timothy Nikkel (:tnikkel)]

Comment on attachment 800428 [details] [diff] [review]
Compute offset of FrameMetrics::mCompositionBounds correctly

>+  nsRect compositionBounds(frameForCompositionBoundsCalculation->GetOffsetToCrossDoc(aReferenceFrame),
>+                           frameForCompositionBoundsCalculation->GetSize());

I realized later that GetSize is probably not what we want. GetSize is for the border rect of the frame, but I think we want the content area of the frame (so that the border of the frame is not including in the composition bounds). We should fix this in a followup.

Comment 11

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tn) from comment #10)
> Comment on attachment 800428 [details] [diff] [review]
> Compute offset of FrameMetrics::mCompositionBounds correctly
> 
> >+  nsRect compositionBounds(frameForCompositionBoundsCalculation->GetOffsetToCrossDoc(aReferenceFrame),
> >+                           frameForCompositionBoundsCalculation->GetSize());
> 
> I realized later that GetSize is probably not what we want. GetSize is for
> the border rect of the frame, but I think we want the content area of the
> frame (so that the border of the frame is not including in the composition
> bounds). We should fix this in a followup.

I filed bug 914666 so we don't forget.

Comment 12

[Botond Ballo [:botond]]

This patch is causing a regression on B2G where http://people.mozilla.org/~kgupta/grid.html cannot be scrolled vertically.

Comment 13

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #12)
> This patch is causing a regression on B2G where
> http://people.mozilla.org/~kgupta/grid.html cannot be scrolled vertically.

The regression is actually caused by the patch for bug 904533. I filed bug 914825 for it.