Closed Bug 913205 Opened 6 years ago Closed 6 years ago

New computation of FrameMetrics::mCompositionBounds causes performance regression

Categories

(Core :: Graphics: Layers, defect)

26 Branch
All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: botond, Assigned: botond)

References

Details

(Keywords: perf, regression)

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 904533
Assignee: nobody → botond
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
Attachment #800410 - Flags: review?(tnikkel)
Attachment #800410 - Flags: feedback?(bugmail.mozilla)
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.
Attachment #800410 - Flags: review?(tnikkel) → review+
Updated patch to address tn's review comments. Carrying r+.
Attachment #800410 - Attachment is obsolete: true
Attachment #800410 - Flags: feedback?(bugmail.mozilla)
Attachment #800428 - Flags: review+
Attachment #800428 - Flags: feedback?(bugmail.mozilla)
Initial talos results are looking good.

Full try job: https://tbpl.mozilla.org/?tree=Try&rev=407963c1063f
Attachment #800428 - Flags: feedback?(bugmail.mozilla) → feedback+
https://hg.mozilla.org/mozilla-central/rev/c4d78e3f9ec1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
(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?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Keywords: perf, regression
Version: Trunk → 26 Branch
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.
(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.
This patch is causing a regression on B2G where http://people.mozilla.org/~kgupta/grid.html cannot be scrolled vertically.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.