Avoid some overhead for backgrounds of elements without border-radius

RESOLVED FIXED in Firefox 55

Status

()

Core
Layout: Web Painting
RESOLVED FIXED
a month ago
28 days ago

People

(Reporter: mstange, Assigned: bas)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a month ago
The stress test at https://people-mozilla.org/~jmuizelaar/implementation-tests/dl-test.html shows 10% of display list building time spent computing the border radii of the affected elements. In this testcase, those radii are all zero.

We can avoid a bit of work by adding a shortcut for that common case.
Comment hidden (mozreview-request)
(Reporter)

Comment 2

a month ago
mozreview-review
Comment on attachment 8871482 [details]
Bug 1367906: Cache the presence of rounded corners upon style changes.

https://reviewboard.mozilla.org/r/142940/#review146666

::: layout/painting/nsCSSRendering.cpp:492
(Diff revision 1)
>  static bool
>  GetRadii(nsIFrame* aForFrame, const nsStyleBorder& aBorder,
>           const nsRect& aOrigBorderArea, const nsRect& aBorderArea,
>           nscoord aRadii[8])
>  {
>    bool haveRoundedCorners;

This is not initialized to false.
Attachment #8871482 - Flags: review-
Comment hidden (mozreview-request)

Comment 4

a month ago
mozreview-review
Comment on attachment 8871482 [details]
Bug 1367906: Cache the presence of rounded corners upon style changes.

https://reviewboard.mozilla.org/r/142940/#review146668

r=dbaron with the following (slightly substantive) changes

::: layout/painting/nsCSSRendering.cpp:497
(Diff revision 1)
> +    if (aForFrame->HasRoundedCorners()) {
> -    haveRoundedCorners = aForFrame->GetBorderRadii(sz, sz, Sides(), aRadii);
> +      haveRoundedCorners = aForFrame->GetBorderRadii(sz, sz, Sides(), aRadii);
> +    }

You either need an |else| here or you need to initialize haveRoundedCorners to false.


However, you also need to fix ConstructBorderRenderer to check the result of GetRadii, since currently it doesn't, and this means it doesn't initialize aRadii.

Or, alternatively, it might be cleaner to just memset the radii to 0 in an else condition.

Could you also add a FIXME: comment to nsIFrame::GetBorderRadii itself that it should probably use this flag in the future.  (The code that computes the flag would then need to use something else, though.)
Attachment #8871482 - Flags: review?(dbaron) → review+

Comment 5

a month ago
mozreview-review
Comment on attachment 8871482 [details]
Bug 1367906: Cache the presence of rounded corners upon style changes.

https://reviewboard.mozilla.org/r/142940/#review146692

::: layout/generic/nsIFrame.h:3907
(Diff revision 2)
>    mozilla::WritingMode mWritingMode;
>  
>    /** The type of the frame. */
>    mozilla::LayoutFrameType mType;
>  
> +  bool mHasRoundedCorners : 1;

Is it worth inverting this (to mNoRoundedCorners) and then lazily initializing it?

That way we don't need to run GetBorderRadii within a style change for a frame that won't be painted.
(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> Is it worth inverting this (to mNoRoundedCorners) and then lazily
> initializing it?

That sounds like a good idea to me.

That would also make it easier to put inside of nsIFrame::GetBorderRadii, perhaps?
(Assignee)

Comment 7

a month ago
mozreview-review
Comment on attachment 8871482 [details]
Bug 1367906: Cache the presence of rounded corners upon style changes.

https://reviewboard.mozilla.org/r/142940/#review146696

::: layout/generic/nsIFrame.h:3907
(Diff revision 2)
>    mozilla::WritingMode mWritingMode;
>  
>    /** The type of the frame. */
>    mozilla::LayoutFrameType mType;
>  
> +  bool mHasRoundedCorners : 1;

David and I discussed something like this, we agreed that since right now we do this for -every- item in the display port -every- frame. It probably for the moment wasn't worth optimizing.

I assume your idea is we set it to true, if it's true we update it inside GetRadii, possibly setting it to false, and in DidGetStyleContext we unconditionally set it to true?

I thought about that for naive callers that don't kno where this is updated it would be incorrect. For the moment I felt having this value be 'correct' and ready for use by future callers without that confusion seemed more worthwhile.
Comment hidden (mozreview-request)

Comment 9

a month ago
mozreview-review
Comment on attachment 8871482 [details]
Bug 1367906: Cache the presence of rounded corners upon style changes.

https://reviewboard.mozilla.org/r/142940/#review146722

::: layout/generic/nsFrame.cpp:1554
(Diff revision 3)
>  nsIFrame::GetBorderRadii(const nsSize& aFrameSize, const nsSize& aBorderArea,
>                           Sides aSkipSides, nscoord aRadii[8]) const

Towards the end of this function you need to set mMayHaveRoundedCorners to false when the result is false.  Otherwise there's no optimization!

::: layout/generic/nsIFrame.h:618
(Diff revision 3)
>      , mParent(nullptr)
>      , mNextSibling(nullptr)
>      , mPrevSibling(nullptr)
>      , mState(NS_FRAME_FIRST_REFLOW | NS_FRAME_IS_DIRTY)
>      , mType(aType)
> +    , mMayHaveRoundedCorners(false)

probably more sensible to initialize to true here, although it doesn't matter since there should always be a DidSetStyleContext immediately after construction.

Comment 10

a month ago
mozreview-review
Comment on attachment 8871482 [details]
Bug 1367906: Cache the presence of rounded corners upon style changes.

https://reviewboard.mozilla.org/r/142940/#review146756

Looks good, with dbaron's comments fixed.
Attachment #8871482 - Flags: review?(matt.woodrow) → review+

Comment 11

29 days ago
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfa432683e27
Cache the presence of rounded corners upon style changes. r=dbaron r=mattwoodrow
(Assignee)

Comment 12

29 days ago
This makes a measurable difference in DisplayList building times which hurt us on a lot of places (Twitter for example).
Whiteboard: [qf:p1]
https://hg.mozilla.org/mozilla-central/rev/bfa432683e27
Status: NEW → RESOLVED
Last Resolved: 28 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.