Changeset 274818 in webkit

Timestamp:

Mar 22, 2021, 4:36:35 PM (5 years ago)

Author:

Patrick Angle

Message:

REGRESSION(r272433): Inspector should not instrument inside WebCore::Node::setRenderer
​https://bugs.webkit.org/show_bug.cgi?id=223559

Reviewed by Ryosuke Niwa and Devin Rousso.

Existing test coverage:

• inspector/css/nodeLayoutContextTypeChanged.html
• inspector/css/setLayoutContextTypeChangedMode.html

The previous approach to observing render changes was most likely a performance regression in a very hot code
path (Node::setRenderer). This patch resolves this by not instrumenting in this the hot path. Instead we call
inspector instrumentation inside the constructors/destructors of only the RenderObject subclasses we are
interested in observing layout changes for.

Additionally, layout change events are now added to a Vector of pending changes, which will be sent to the
front-end later in order to avoid evaluating JavaScript inside a destructor in WK1 with the new instrumentation
points.

• dom/Element.cpp:

(WebCore::Element::didChangeRenderer): Deleted.

• dom/Element.h:
• dom/Node.h:

(WebCore::Node::didChangeRenderer): Deleted.

• inspector/InspectorInstrumentation.cpp:

(WebCore::InspectorInstrumentation::nodeLayoutContextChangedImpl):

• inspector/InspectorInstrumentation.h:

(WebCore::InspectorInstrumentation::nodeLayoutContextChanged):

• inspector/agents/InspectorCSSAgent.cpp:

(WebCore::InspectorCSSAgent::InspectorCSSAgent):
(WebCore::InspectorCSSAgent::reset):
(WebCore::InspectorCSSAgent::nodeLayoutContextTypeChanged):
(WebCore::InspectorCSSAgent::layoutContextTypeChangedTimerFired):

• Moved layout change events behind a timer firing.
• inspector/agents/InspectorCSSAgent.h:
• rendering/RenderGrid.cpp:

(WebCore::RenderGrid::RenderGrid):
(WebCore::RenderGrid::~RenderGrid):

• Move instrumentation from Node::setRenderer to RenderGrid.
• rendering/RenderObject.h:

(WebCore::Node::setRenderer):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• dom/Element.cpp (modified) (1 diff)
• dom/Element.h (modified) (1 diff)
• dom/Node.h (modified) (1 diff)
• inspector/InspectorInstrumentation.cpp (modified) (1 diff)
• inspector/InspectorInstrumentation.h (modified) (1 diff)
• inspector/agents/InspectorCSSAgent.cpp (modified) (4 diffs)
• inspector/agents/InspectorCSSAgent.h (modified) (3 diffs)
• rendering/RenderGrid.cpp (modified) (2 diffs)
• rendering/RenderObject.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-03-22  Patrick Angle  <pangle@apple.com>
+
+        REGRESSION(r272433): Inspector should not instrument inside `WebCore::Node::setRenderer`
+        https://bugs.webkit.org/show_bug.cgi?id=223559
+
+        Reviewed by Ryosuke Niwa and Devin Rousso.
+
+        Existing test coverage:
+        - inspector/css/nodeLayoutContextTypeChanged.html
+        - inspector/css/setLayoutContextTypeChangedMode.html
+
+        The previous approach to observing render changes was most likely a performance regression in a very hot code
+        path (`Node::setRenderer`). This patch resolves this by not instrumenting in this the hot path. Instead we call
+        inspector instrumentation inside the constructors/destructors of only the RenderObject subclasses we are
+        interested in observing layout changes for.
+
+        Additionally, layout change events are now added to a `Vector` of pending changes, which will be sent to the
+        front-end later in order to avoid evaluating JavaScript inside a destructor in WK1 with the new instrumentation
+        points.
+
+        * dom/Element.cpp:
+        (WebCore::Element::didChangeRenderer): Deleted.
+        * dom/Element.h:
+        * dom/Node.h:
+        (WebCore::Node::didChangeRenderer): Deleted.
+        * inspector/InspectorInstrumentation.cpp:
+        (WebCore::InspectorInstrumentation::nodeLayoutContextChangedImpl):
+        * inspector/InspectorInstrumentation.h:
+        (WebCore::InspectorInstrumentation::nodeLayoutContextChanged):
+        * inspector/agents/InspectorCSSAgent.cpp:
+        (WebCore::InspectorCSSAgent::InspectorCSSAgent):
+        (WebCore::InspectorCSSAgent::reset):
+        (WebCore::InspectorCSSAgent::nodeLayoutContextTypeChanged):
+        (WebCore::InspectorCSSAgent::layoutContextTypeChangedTimerFired):
+        - Moved layout change events behind a timer firing.
+        * inspector/agents/InspectorCSSAgent.h:
+        * rendering/RenderGrid.cpp:
+        (WebCore::RenderGrid::RenderGrid):
+        (WebCore::RenderGrid::~RenderGrid):
+        - Move instrumentation from `Node::setRenderer` to `RenderGrid`.
+        * rendering/RenderObject.h:
+        (WebCore::Node::setRenderer):
+
 2021-03-22  Peng Liu  <peng.liu6@apple.com>
 

trunk/Source/WebCore/dom/Element.cpp

@@ -4595 +4595 @@
 }
 
-void Element::didChangeRenderer(RenderObject* oldRenderer)
-{
-    InspectorInstrumentation::nodeLayoutContextChanged(*this, oldRenderer);
-}
-
 #if ENABLE(CSS_TYPED_OM)
 

trunk/Source/WebCore/dom/Element.h

@@ -734 +734 @@
     void attachAttributeNodeIfNeeded(Attr&);
     
-    void didChangeRenderer(RenderObject*) final;
-
 #if ASSERT_ENABLED
     WEBCORE_EXPORT bool fastAttributeLookupAllowed(const QualifiedName&) const;

trunk/Source/WebCore/dom/Node.h

@@ -711 +711 @@
     void moveNodeToNewDocument(Document& oldDocument, Document& newDocument);
     
-    virtual void didChangeRenderer(RenderObject*) { };
-
     struct NodeRareDataDeleter {
         void operator()(NodeRareData*) const;

trunk/Source/WebCore/inspector/InspectorInstrumentation.cpp

@@ -177 +177 @@
 }
 
-void InspectorInstrumentation::nodeLayoutContextChangedImpl(InstrumentingAgents& instrumentingAgents, Node& node, RenderObject* oldRenderer)
+void InspectorInstrumentation::nodeLayoutContextChangedImpl(InstrumentingAgents& instrumentingAgents, Node& node, RenderObject* newRenderer)
 {
     if (auto* cssAgent = instrumentingAgents.enabledCSSAgent())
-        cssAgent->nodeLayoutContextTypeChanged(node, oldRenderer); 
+        cssAgent->nodeLayoutContextTypeChanged(node, newRenderer);
 }
 

trunk/Source/WebCore/inspector/InspectorInstrumentation.h

@@ -602 +602 @@
 }
 
-inline void InspectorInstrumentation::nodeLayoutContextChanged(Node& node, RenderObject* oldRenderer)
+inline void InspectorInstrumentation::nodeLayoutContextChanged(Node& node, RenderObject* newRenderer)
 {
     FAST_RETURN_IF_NO_FRONTENDS(void());
     if (auto* agents = instrumentingAgents(node.document()))
-        nodeLayoutContextChangedImpl(*agents, node, oldRenderer);
+        nodeLayoutContextChangedImpl(*agents, node, newRenderer);
 }
 

trunk/Source/WebCore/inspector/agents/InspectorCSSAgent.cpp

@@ -305 +305 @@
     , m_frontendDispatcher(makeUnique<CSSFrontendDispatcher>(context.frontendRouter))
     , m_backendDispatcher(CSSBackendDispatcher::create(context.backendDispatcher, this))
+    , m_layoutContextTypeChangedTimer(*this, &InspectorCSSAgent::layoutContextTypeChangedTimerFired)
 {
 }
@@ -327 +328 @@
     m_documentToInspectorStyleSheet.clear();
     m_documentToKnownCSSStyleSheets.clear();
+    m_nodesWithPendingLayoutContextTypeChanges.clear();
+    if (m_layoutContextTypeChangedTimer.isActive())
+        m_layoutContextTypeChangedTimer.stop();
     m_layoutContextTypeChangedMode = Protocol::CSS::LayoutContextTypeChangedMode::Observed;
     resetPseudoStates();
@@ -967 +971 @@
 }
 
-void InspectorCSSAgent::nodeLayoutContextTypeChanged(Node& node, RenderObject* oldRenderer)
+void InspectorCSSAgent::nodeLayoutContextTypeChanged(Node& node, RenderObject* newRenderer)
 {
     auto* domAgent = m_instrumentingAgents.persistentDOMAgent();
     if (!domAgent)
         return;
-    
-    auto newLayoutContextType = layoutContextTypeForRenderer(node.renderer());
-    if (newLayoutContextType == layoutContextTypeForRenderer(oldRenderer))
-        return;
-    
+
     auto nodeId = domAgent->boundNodeId(&node);
     if (!nodeId && m_layoutContextTypeChangedMode == Protocol::CSS::LayoutContextTypeChangedMode::All) {
@@ -984 +984 @@
     if (!nodeId)
         return;
-    
-    m_frontendDispatcher->nodeLayoutContextTypeChanged(nodeId, WTFMove(newLayoutContextType));
+
+    m_nodesWithPendingLayoutContextTypeChanges.set(nodeId, layoutContextTypeForRenderer(newRenderer));
+    if (!m_layoutContextTypeChangedTimer.isActive())
+        m_layoutContextTypeChangedTimer.startOneShot(0_s);
+}
+
+void InspectorCSSAgent::layoutContextTypeChangedTimerFired()
+{
+    for (auto&& [nodeId, layoutContextType] : std::exchange(m_nodesWithPendingLayoutContextTypeChanges, { }))
+        m_frontendDispatcher->nodeLayoutContextTypeChanged(nodeId, WTFMove(layoutContextType));
 }
 

trunk/Source/WebCore/inspector/agents/InspectorCSSAgent.h

@@ -31 +31 @@
 #include "InspectorWebAgentBase.h"
 #include "SecurityContext.h"
+#include "Timer.h"
 #include <JavaScriptCore/InspectorBackendDispatchers.h>
 #include <wtf/HashMap.h>
@@ -157 +158 @@
     RefPtr<Inspector::Protocol::CSS::CSSStyle> buildObjectForAttributesStyle(StyledElement&);
 
+    void layoutContextTypeChangedTimerFired();
 
     void resetPseudoStates();
@@ -173 +175 @@
     int m_lastStyleSheetId { 1 };
     bool m_creatingViaInspectorStyleSheet { false };
+
+    HashMap<Inspector::Protocol::DOM::NodeId, Optional<Inspector::Protocol::CSS::LayoutContextType>> m_nodesWithPendingLayoutContextTypeChanges;
+    Timer m_layoutContextTypeChangedTimer;
     Inspector::Protocol::CSS::LayoutContextTypeChangedMode m_layoutContextTypeChangedMode { Inspector::Protocol::CSS::LayoutContextTypeChangedMode::Observed };
 };

trunk/Source/WebCore/rendering/RenderGrid.cpp

@@ -32 +32 @@
 #include "GridPositionsResolver.h"
 #include "GridTrackSizingAlgorithm.h"
+#include "InspectorInstrumentation.h"
 #include "LayoutRepainter.h"
 #include "RenderChildIterator.h"
@@ -57 +58 @@
     // All of our children must be block level.
     setChildrenInline(false);
-}
-
-RenderGrid::~RenderGrid() = default;
+
+    InspectorInstrumentation::nodeLayoutContextChanged(element, this);
+}
+
+RenderGrid::~RenderGrid()
+{
+    InspectorInstrumentation::nodeLayoutContextChanged(element(), nullptr);
+}
 
 StyleSelfAlignmentData RenderGrid::selfAlignmentForChild(GridAxis axis, const RenderBox& child, const RenderStyle* gridStyle) const

trunk/Source/WebCore/rendering/RenderObject.h

@@ -1151 +1151 @@
 inline void Node::setRenderer(RenderObject* renderer)
 {
-    auto oldRenderer = this->renderer();
     m_rendererWithStyleFlags.setPointer(renderer);
-    didChangeRenderer(oldRenderer);
 }