1367906 - Avoid some overhead for backgrounds of elements without border-radius

Status: RESOLVED FIXED

(Core :: Web Painting, enhancement)

Description

[Markus Stange [:mstange]]

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 1

[Bas Schouten (:bas.schouten)]

Attachment: Bug 1367906: Cache the presence of rounded corners upon style changes.

Profiling indicates a significant amount of time is spent during displaylist building accessing style information in order to detect border radii. This is a cost that for frames with no border radii only needs to be paid when the style changes.

Since nsIFrame still has 16 bits available for storing data without growing it in size, this patch recomputes whether there are border radii upon DidGetStyleContext and stores that information in one of these unused bits.

Review commit: https://reviewboard.mozilla.org/r/142940/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142940/

Comment 2

[Markus Stange [:mstange]]

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.

Comment 3

[Bas Schouten (:bas.schouten)]

Comment on attachment 8871482 [details]
Bug 1367906: Cache the presence of rounded corners upon style changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142940/diff/1-2/

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 5

[Matt Woodrow (:mattwoodrow)]

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.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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 7

[Bas Schouten (:bas.schouten)]

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 8

[Bas Schouten (:bas.schouten)]

Comment on attachment 8871482 [details]
Bug 1367906: Cache the presence of rounded corners upon style changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142940/diff/2-3/

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[Matt Woodrow (:mattwoodrow)]

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.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfa432683e27
Cache the presence of rounded corners upon style changes. r=dbaron r=mattwoodrow

Comment 12

[Bas Schouten (:bas.schouten)]

This makes a measurable difference in DisplayList building times which hurt us on a lot of places (Twitter for example).

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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