Closed Bug 1367906 Opened 3 years ago Closed 3 years ago

Avoid some overhead for backgrounds of elements without border-radius

Categories

(Core :: Web Painting, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: mstange, Assigned: bas.schouten)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qf:p1])

Attachments

(1 file)

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 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 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 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?
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 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 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+
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
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
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.