Bug 231016

Summary:

[Performance] Optimize RenderLayer::establishesTopLayer

Product:

WebKit

Reporter:

cathiechen <cathiechen>

Component:

Layout and Rendering

Assignee:

cathiechen <cathiechen>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, cdumez, changseok, cmarcelo, esprehn+autocc, ews-watchlist, fred.wang, glenn, kangil.han, kondapallykalyan, ntim, pdr, rbuis, simon.fraser, webkit-bug-importer, zalan

Priority:

Keywords:

InRadar

Version:

WebKit Local Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=229802

Attachments:

Description	Flags
Patch	none
screenshot-of-original-establishesTopLayer	none
screenshot-of-using-RefPtr-in-establishesTopLayer	none
screenshot-of-using-Element-point-in-establishesTopLayer	none
Patch	none
Patch	none

Description cathiechen 2021-09-30 02:57:27 PDT

Reduce the call of renderer().element()

Comment 1 cathiechen 2021-09-30 03:04:38 PDT

Created attachment 439714 [details]
Patch

Comment 2 cathiechen 2021-09-30 03:07:07 PDT

Created attachment 439715 [details]
screenshot-of-original-establishesTopLayer

Comment 3 cathiechen 2021-09-30 03:10:04 PDT

Created attachment 439717 [details]
screenshot-of-using-RefPtr-in-establishesTopLayer

Comment 4 cathiechen 2021-09-30 03:11:42 PDT

Created attachment 439718 [details]
screenshot-of-using-Element-point-in-establishesTopLayer

Comment 5 cathiechen 2021-09-30 03:13:30 PDT

Test locally, here are the results:
* Original: 4.9%, 5.5%, 4.7%
* RefPtr: 5.1%, 4.7%, 5.0%
* Element*: 3.5%, 3.5%, 3.4%

When using RefPtr, there's extra cost to maintain it.
So using Element* directly in the patch.

Comment 6 cathiechen 2021-09-30 03:14:59 PDT

(In reply to cathiechen from comment #5)
> Test locally, here are the results:
> * Original: 4.9%, 5.5%, 4.7%
> * RefPtr: 5.1%, 4.7%, 5.0%
> * Element*: 3.5%, 3.5%, 3.4%
> 
> When using RefPtr, there's extra cost to maintain it.
> So using Element* directly in the patch.

The results come from the test case in bug 229802.

Comment 7 Simon Fraser (smfr) 2021-09-30 10:07:47 PDT

Comment on attachment 439714 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439714&action=review

> Source/WebCore/rendering/RenderLayer.cpp:3959
> -    return renderer().element()->isInTopLayer();
> +    return renderer().style().styleType() == PseudoId::Backdrop;

This is similar to code in Adjuster::adjust(): 
        bool isInTopLayer = style.styleType() == PseudoId::Backdrop || (m_element && m_element->isInTopLayer());

Can we share the code?

Comment 8 cathiechen 2021-09-30 19:58:09 PDT

Comment on attachment 439714 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439714&action=review

>> Source/WebCore/rendering/RenderLayer.cpp:3959
>> +    return renderer().style().styleType() == PseudoId::Backdrop;
> 
> This is similar to code in Adjuster::adjust(): 
>         bool isInTopLayer = style.styleType() == PseudoId::Backdrop || (m_element && m_element->isInTopLayer());
> 
> Can we share the code?

Add a static function to Element.h?
static bool isInTopLayerOrBackdrop(const Element&, const RenderStyle&)

Comment 9 cathiechen 2021-09-30 20:00:20 PDT

(In reply to cathiechen from comment #8)
> Comment on attachment 439714 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=439714&action=review
> 
> >> Source/WebCore/rendering/RenderLayer.cpp:3959
> >> +    return renderer().style().styleType() == PseudoId::Backdrop;
> > 
> > This is similar to code in Adjuster::adjust(): 
> >         bool isInTopLayer = style.styleType() == PseudoId::Backdrop || (m_element && m_element->isInTopLayer());
> > 
> > Can we share the code?
> 
> Add a static function to Element.h?
> static bool isInTopLayerOrBackdrop(const Element&, const RenderStyle&)

Oh, element could be null. So

static bool isInTopLayerOrBackdrop(Element*, const RenderStyle&)

Comment 10 Simon Fraser (smfr) 2021-09-30 20:11:34 PDT

Comment on attachment 439714 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439714&action=review

>>>> Source/WebCore/rendering/RenderLayer.cpp:3959
>>>> +    return renderer().style().styleType() == PseudoId::Backdrop;
>>> 
>>> This is similar to code in Adjuster::adjust(): 
>>>         bool isInTopLayer = style.styleType() == PseudoId::Backdrop || (m_element && m_element->isInTopLayer());
>>> 
>>> Can we share the code?
>> 
>> Add a static function to Element.h?
>> static bool isInTopLayerOrBackdrop(const Element&, const RenderStyle&)
> 
> Oh, element could be null. So
> 
> static bool isInTopLayerOrBackdrop(Element*, const RenderStyle&)

static bool isInTopLayerOrBackdrop(Element*, const RenderStyle&) seems OK.

Comment 11 cathiechen 2021-09-30 20:39:26 PDT

Comment on attachment 439714 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439714&action=review

>>>>> Source/WebCore/rendering/RenderLayer.cpp:3959
>>>>> +    return renderer().style().styleType() == PseudoId::Backdrop;
>>>> 
>>>> This is similar to code in Adjuster::adjust(): 
>>>>         bool isInTopLayer = style.styleType() == PseudoId::Backdrop || (m_element && m_element->isInTopLayer());
>>>> 
>>>> Can we share the code?
>>> 
>>> Add a static function to Element.h?
>>> static bool isInTopLayerOrBackdrop(const Element&, const RenderStyle&)
>> 
>> Oh, element could be null. So
>> 
>> static bool isInTopLayerOrBackdrop(Element*, const RenderStyle&)
> 
> static bool isInTopLayerOrBackdrop(Element*, const RenderStyle&) seems OK.

OK, I'll have a try:) The extra call of renderer().style() might affect the result a little bit.

Comment 12 cathiechen 2021-10-01 01:19:18 PDT

Comment on attachment 439714 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439714&action=review

>>>>>> Source/WebCore/rendering/RenderLayer.cpp:3959
>>>>>> +    return renderer().style().styleType() == PseudoId::Backdrop;
>>>>> 
>>>>> This is similar to code in Adjuster::adjust(): 
>>>>>         bool isInTopLayer = style.styleType() == PseudoId::Backdrop || (m_element && m_element->isInTopLayer());
>>>>> 
>>>>> Can we share the code?
>>>> 
>>>> Add a static function to Element.h?
>>>> static bool isInTopLayerOrBackdrop(const Element&, const RenderStyle&)
>>> 
>>> Oh, element could be null. So
>>> 
>>> static bool isInTopLayerOrBackdrop(Element*, const RenderStyle&)
>> 
>> static bool isInTopLayerOrBackdrop(Element*, const RenderStyle&) seems OK.
> 
> OK, I'll have a try:) The extra call of renderer().style() might affect the result a little bit.

Looks like the result doesn't change much. Here are the results based on the patch of bug 230885.

* isInTopLayerOrBackdrop: 0.7%, 1.1%, 0.9%
* Element*: 0.8%, 0.8%, 0.6%

Comment 13 cathiechen 2021-10-01 01:37:05 PDT

Created attachment 439830 [details]
Patch

Comment 14 Simon Fraser (smfr) 2021-10-01 10:43:27 PDT

Comment on attachment 439830 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439830&action=review

> Source/WebCore/dom/Element.h:512
> +    static bool isInTopLayerOrBackdrop(const RenderStyle& style, const Element* element)
> +    {
> +        return (element && element->isInTopLayer()) || style.styleType() == PseudoId::Backdrop;
> +    }

I would make this an inline free function at the bottom of this header.

Comment 15 cathiechen 2021-10-01 16:26:49 PDT

Created attachment 439929 [details]
Patch

Comment 16 EWS 2021-10-02 06:40:27 PDT

Committed r283441 (242428@main): <https://commits.webkit.org/242428@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439929 [details].

Comment 17 Radar WebKit Bug Importer 2021-10-02 06:41:16 PDT

<rdar://problem/83797706>