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

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.