231016 – [Performance] Optimize RenderLayer::establishesTopLayer

RESOLVED FIXED 231016

[Performance] Optimize RenderLayer::establishesTopLayer

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

Summary [Performance] Optimize RenderLayer::establishesTopLayer

cathiechen

Reported 2021-09-30 02:57:27 PDT

Reduce the call of renderer().element()

Attachments
Patch (1.55 KB, patch) 2021-09-30 03:04 PDT, cathiechen	no flags	Details Formatted Diff Diff
screenshot-of-original-establishesTopLayer (625.33 KB, image/png) 2021-09-30 03:07 PDT, cathiechen	no flags	Details
screenshot-of-using-RefPtr-in-establishesTopLayer (1005.36 KB, image/png) 2021-09-30 03:10 PDT, cathiechen	no flags	Details
screenshot-of-using-Element-point-in-establishesTopLayer (879.59 KB, image/png) 2021-09-30 03:11 PDT, cathiechen	no flags	Details
Patch (4.12 KB, patch) 2021-10-01 01:37 PDT, cathiechen	no flags	Details Formatted Diff Diff
Patch (3.96 KB, patch) 2021-10-01 16:26 PDT, cathiechen	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

cathiechen

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

Created attachment 439714 [details] Patch

cathiechen

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

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

cathiechen

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

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

cathiechen

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

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

cathiechen

Comment 5 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.

cathiechen

Comment 6 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.

Simon Fraser (smfr)

Comment 7 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?

cathiechen

Comment 8 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&)

cathiechen

Comment 9 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&)

Simon Fraser (smfr)

Comment 10 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.

cathiechen

Comment 11 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.

cathiechen

Comment 12 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%

cathiechen

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

Created attachment 439830 [details] Patch

Simon Fraser (smfr)

Comment 14 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.

cathiechen

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

Created attachment 439929 [details] Patch

EWS

Comment 16 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].

Radar WebKit Bug Importer

Comment 17 2021-10-02 06:41:16 PDT

<rdar://problem/83797706>

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Local Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component Layout and Rendering

Assignee

cathiechen

Reported

2021-09-30 02:57 PDT

Modified

2021-10-02 06:41 PDT History

CC List

16 users Show

URL

Keywords InRadar

Depends on

Blocks