Reduce the call of renderer().element()
Created attachment 439714 [details] Patch
Created attachment 439715 [details] screenshot-of-original-establishesTopLayer
Created attachment 439717 [details] screenshot-of-using-RefPtr-in-establishesTopLayer
Created attachment 439718 [details] screenshot-of-using-Element-point-in-establishesTopLayer
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.
(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 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 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&)
(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 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 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 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%
Created attachment 439830 [details] Patch
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.
Created attachment 439929 [details] Patch
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].
<rdar://problem/83797706>