[LayoutNG] Reduce excessive marking of line box dirtiness

This patch fixes two excessive marking of line box dirtiness
in NGInlineNode::MarkLineBoxesDirty().

1. LayoutObject::InsertedIntoTree() marks dirty, even if it
   will be dirtied again later in NGInlineNode::
   MarkLineBoxesDirty(). Fixed LayoutNGMixin<Base>::
   DirtyLinesFromChangedChild() to do only if it was in NG
   inline formatting context.
2. It may dirty the first line box because it clears
   LayoutObject::FirstInlineFragment as it traverses in pre-
   order, but NGPaintFragment::DirtyLinesFromChangedChild()
   needs FirstInlineFragment of its previous siblings.
   Changed to keep previous fragment in this loop.

blink_perf.layout does not show observable improvements, but
loading.desktop shows ~3% improvements.

Bug: 636993
Change-Id: Ia0706bb2407427b3e4ed093290646eb21bc5c0f3
Reviewed-on: https://chromium-review.googlesource.com/c/1309388
Commit-Queue: Koji Ishii <kojii@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604835}

third_party/blink/renderer/core/layout/layout_text.cc

@@ -218,7 +218,7 @@ void LayoutText::RemoveAndDestroyTextBoxes() {
         box->Remove();
     } else if (Parent()) {
       if (NGPaintFragment* first_fragment = FirstInlineFragment())
-        first_fragment->MarkLineBoxDirty();
+        first_fragment->MarkContainingLineBoxDirty();
       else
         Parent()->DirtyLinesFromChangedChild(this);
     }

third_party/blink/renderer/core/layout/ng/inline/ng_inline_node.cc

@@ -719,17 +719,16 @@ bool NGInlineNode::PrepareReuseFragments(
   return true;
 }
 
-// Mark line boxes dirty for where marked for |NeedsLayout()|.
+// Mark the first line box that have |NeedsLayout()| dirty.
 //
-// Removal of LayoutObject already marks relevant line boxes dirty by calling
-// |DirtyLinesFromChangedChild()|, but style changes have not marked yet.
-//
-// TODO(kojii): By having this loop, we probably don't have to mark in
-// |InsertedIntoTree()|. NG mimics legacy doing so in |InsertedIntoTree()| and
-// also have this loop in |LayoutBlockFlow::LayoutInlineChildren()|. Investigate
-// if we can remove the duplicated marking.
+// Removals of LayoutObject already marks relevant line boxes dirty by calling
+// |DirtyLinesFromChangedChild()|, but insertions and style changes are not
+// marked yet.
 bool NGInlineNode::MarkLineBoxesDirty(LayoutBlockFlow* block_flow) {
+  DCHECK(block_flow);
+  DCHECK(block_flow->PaintFragment());
   bool has_dirtied_lines = false;
+  NGPaintFragment* last_fragment = nullptr;
   for (LayoutObject* layout_object = block_flow->NextInPreOrder(block_flow);
        layout_object;) {
     bool should_dirty_lines = false;
@@ -744,7 +743,7 @@ bool NGInlineNode::MarkLineBoxesDirty(LayoutBlockFlow* block_flow) {
           !has_dirtied_lines && layout_object->SelfNeedsLayout();
       next = layout_object->NextInPreOrder(block_flow);
       layout_object->ClearNeedsLayout();
-    } else if (layout_object->IsFloatingOrOutOfFlowPositioned()) {
+    } else if (UNLIKELY(layout_object->IsFloatingOrOutOfFlowPositioned())) {
       // Aborting in the middle of the traversal is safe because this function
       // ClearNeedsLayout() on text and LayoutInline, but since an inline
       // formatting context is laid out as a whole, these flags don't matter.
@@ -764,13 +763,30 @@ bool NGInlineNode::MarkLineBoxesDirty(LayoutBlockFlow* block_flow) {
       // crbug.com/897141
       next = layout_object->NextInPreOrder(block_flow);
     }
-    if (should_dirty_lines) {
-      // TODO(kojii): This seems to invalidate more than necessary because this
-      // function reads PreviousSibling, whose InlineFragments maybe cleared
-      // already.
-      NGPaintFragment::DirtyLinesFromChangedChild(layout_object);
-      has_dirtied_lines = true;
+
+    if (!has_dirtied_lines) {
+      if (should_dirty_lines) {
+        if (last_fragment) {
+          // Changes in this LayoutObject may affect the line that contains its
+          // previous object. Mark the line box that contains the last fragment
+          // of the previous object.
+          last_fragment->LastForSameLayoutObject()
+              ->MarkContainingLineBoxDirty();
+        } else {
+          // If there were no fragments so far in this pre-order traversal, mark
+          // the first line box dirty.
+          NGPaintFragment* block_fragment = block_flow->PaintFragment();
+          DCHECK(block_fragment);
+          if (NGPaintFragment* first_line = block_fragment->FirstLineBox())
+            first_line->MarkLineBoxDirty();
+        }
+        has_dirtied_lines = true;
+      } else {
+        if (NGPaintFragment* fragment = layout_object->FirstInlineFragment())
+          last_fragment = fragment;
+      }
     }
+
     ClearInlineFragment(layout_object);
     layout_object = next;
   }

third_party/blink/renderer/core/layout/ng/inline/ng_inline_node_test.cc

@@ -16,6 +16,7 @@
 #include "third_party/blink/renderer/core/layout/ng/ng_layout_result.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_layout_test.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_physical_box_fragment.h"
+#include "third_party/blink/renderer/core/paint/ng/ng_paint_fragment.h"
 #include "third_party/blink/renderer/core/style/computed_style.h"
 
 namespace blink {
@@ -67,6 +68,10 @@ class NGInlineNodeForTest : public NGInlineNode {
 
   void CollectInlines() { NGInlineNode::CollectInlines(MutableData()); }
   void ShapeText() { NGInlineNode::ShapeText(MutableData()); }
+
+  bool MarkLineBoxesDirty() {
+    return NGInlineNode::MarkLineBoxesDirty(GetLayoutBlockFlow());
+  }
 };
 
 class NGInlineNodeTest : public NGLayoutTest {
@@ -127,6 +132,24 @@ class NGInlineNodeTest : public NGLayoutTest {
     return data->text_content;
   }
 
+  // Mark line boxes dirty and returns child paint fragments of
+  // |layout_block_flow_|.
+  Vector<NGPaintFragment*, 16> MarkLineBoxesDirty() const {
+    // Attach new LayoutObjects if there were any, but do not run layout,
+    // because running layout will re-create fragments.
+    GetDocument().UpdateStyleAndLayoutTree();
+
+    NGInlineNodeForTest node(layout_block_flow_);
+    EXPECT_TRUE(node.MarkLineBoxesDirty());
+
+    scoped_refptr<const NGPaintFragment> fragment =
+        layout_block_flow_->PaintFragment();
+    EXPECT_TRUE(fragment);
+    Vector<NGPaintFragment*, 16> children;
+    fragment->Children().ToList(&children);
+    return children;
+  }
+
   Vector<NGInlineItem>& Items() {
     NGInlineNodeData* data = layout_block_flow_->GetNGInlineNodeData();
     CHECK(data);
@@ -645,4 +668,166 @@ TEST_F(NGInlineNodeTest, SpaceRestoredByInsertingWord) {
   EXPECT_EQ(String("before mid after"), GetText());
 }
 
+// Test marking line boxes when inserting a span before the first child.
+TEST_F(NGInlineNodeTest, MarkLineBoxesDirtyOnInsert) {
+  SetupHtml("container", R"HTML(
+    <div id=container style="font-size: 10px; width: 10ch">
+      12345678
+    </div>
+  )HTML");
+
+  Element* span = GetDocument().CreateElementForBinding("span");
+  Element* container = GetElementById("container");
+  container->insertBefore(span, container->firstChild());
+
+  auto lines = MarkLineBoxesDirty();
+  EXPECT_TRUE(lines[0]->IsDirty());
+}
+
+// Test marking line boxes when appending a span.
+TEST_F(NGInlineNodeTest, MarkLineBoxesDirtyOnAppend) {
+  SetupHtml("container", R"HTML(
+    <div id=container style="font-size: 10px; width: 10ch">
+      12345678
+    </div>
+  )HTML");
+
+  Element* span = GetDocument().CreateElementForBinding("span");
+  layout_block_flow_->GetNode()->appendChild(span);
+
+  auto lines = MarkLineBoxesDirty();
+  EXPECT_TRUE(lines[0]->IsDirty());
+}
+
+// Test marking line boxes when appending a span on 2nd line.
+TEST_F(NGInlineNodeTest, MarkLineBoxesDirtyOnAppend2) {
+  SetupHtml("container", R"HTML(
+    <div id=container style="font-size: 10px; width: 10ch">
+      12345678
+      2234
+    </div>
+  )HTML");
+
+  Element* span = GetDocument().CreateElementForBinding("span");
+  layout_block_flow_->GetNode()->appendChild(span);
+
+  auto lines = MarkLineBoxesDirty();
+  EXPECT_FALSE(lines[0]->IsDirty());
+  EXPECT_TRUE(lines[1]->IsDirty());
+}
+
+// Test marking line boxes when removing a span.
+TEST_F(NGInlineNodeTest, MarkLineBoxesDirtyOnRemove) {
+  SetupHtml("container", R"HTML(
+    <div id=container style="font-size: 10px; width: 10ch">
+      1234<span id=t>5678</span>
+    </div>
+  )HTML");
+
+  Element* span = GetElementById("t");
+  span->remove();
+
+  auto lines = MarkLineBoxesDirty();
+  EXPECT_TRUE(lines[0]->IsDirty());
+}
+
+// Test marking line boxes when removing a span.
+TEST_F(NGInlineNodeTest, MarkLineBoxesDirtyOnRemoveFirst) {
+  SetupHtml("container", R"HTML(
+    <div id=container style="font-size: 10px; width: 10ch">
+      <span id=t>1234</span>5678
+    </div>
+  )HTML");
+
+  Element* span = GetElementById("t");
+  span->remove();
+
+  auto lines = MarkLineBoxesDirty();
+  EXPECT_TRUE(lines[0]->IsDirty());
+}
+
+// Test marking line boxes when removing a span on 2nd line.
+TEST_F(NGInlineNodeTest, MarkLineBoxesDirtyOnRemove2) {
+  SetupHtml("container", R"HTML(
+    <div id=container style="font-size: 10px; width: 10ch">
+      12345678
+      2234<span id=t>5678 3334</span>
+    </div>
+  )HTML");
+
+  Element* span = GetElementById("t");
+  span->remove();
+
+  auto lines = MarkLineBoxesDirty();
+  EXPECT_FALSE(lines[0]->IsDirty());
+  EXPECT_TRUE(lines[1]->IsDirty());
+}
+
+// Test marking line boxes when the first span has NeedsLayout. The span is
+// culled.
+TEST_F(NGInlineNodeTest, MarkLineBoxesDirtyOnNeedsLayoutFirst) {
+  SetupHtml("container", R"HTML(
+    <div id=container style="font-size: 10px; width: 10ch">
+      <span id=t>1234</span>5678
+    </div>
+  )HTML");
+
+  LayoutObject* span = GetLayoutObjectByElementId("t");
+  span->SetNeedsLayout("");
+
+  auto lines = MarkLineBoxesDirty();
+  EXPECT_TRUE(lines[0]->IsDirty());
+}
+
+// Test marking line boxes when the first span has NeedsLayout. The span has a
+// box fragment.
+TEST_F(NGInlineNodeTest, MarkLineBoxesDirtyOnNeedsLayoutFirstWithBox) {
+  SetupHtml("container", R"HTML(
+    <div id=container style="font-size: 10px; width: 10ch">
+      <span id=t style="background: blue">1234</span>5678
+    </div>
+  )HTML");
+
+  LayoutObject* span = GetLayoutObjectByElementId("t");
+  span->SetNeedsLayout("");
+
+  auto lines = MarkLineBoxesDirty();
+  EXPECT_TRUE(lines[0]->IsDirty());
+}
+
+// Test marking line boxes when a span has NeedsLayout. The span is culled.
+TEST_F(NGInlineNodeTest, MarkLineBoxesDirtyOnNeedsLayout) {
+  SetupHtml("container", R"HTML(
+    <div id=container style="font-size: 10px; width: 10ch">
+      12345678
+      2234<span id=t>5678 3334</span>
+    </div>
+  )HTML");
+
+  LayoutObject* span = GetLayoutObjectByElementId("t");
+  span->SetNeedsLayout("");
+
+  auto lines = MarkLineBoxesDirty();
+  EXPECT_FALSE(lines[0]->IsDirty());
+  EXPECT_TRUE(lines[1]->IsDirty());
+}
+
+// Test marking line boxes when a span has NeedsLayout. The span has a box
+// fragment.
+TEST_F(NGInlineNodeTest, MarkLineBoxesDirtyOnNeedsLayoutWithBox) {
+  SetupHtml("container", R"HTML(
+    <div id=container style="font-size: 10px; width: 10ch">
+      12345678
+      2234<span id=t style="background: blue">5678 3334</span>
+    </div>
+  )HTML");
+
+  LayoutObject* span = GetLayoutObjectByElementId("t");
+  span->SetNeedsLayout("");
+
+  auto lines = MarkLineBoxesDirty();
+  EXPECT_FALSE(lines[0]->IsDirty());
+  EXPECT_TRUE(lines[1]->IsDirty());
+}
+
 }  // namespace blink

third_party/blink/renderer/core/layout/ng/layout_ng_mixin.cc

@@ -429,7 +429,12 @@ void LayoutNGMixin<Base>::DirtyLinesFromChangedChild(
     LayoutObject* child,
     MarkingBehavior marking_behavior) {
   DCHECK_EQ(marking_behavior, kMarkContainerChain);
-  NGPaintFragment::DirtyLinesFromChangedChild(child);
+
+  // We need to dirty line box fragments only if the child is once laid out in
+  // LayoutNG inline formatting context. New objects are handled in
+  // NGInlineNode::MarkLineBoxesDirty().
+  if (child->IsInLayoutNGInlineFormattingContext())
+    NGPaintFragment::DirtyLinesFromChangedChild(child);
 }
 
 template class CORE_TEMPLATE_EXPORT LayoutNGMixin<LayoutTableCaption>;

third_party/blink/renderer/core/paint/ng/ng_paint_fragment.cc

@@ -628,7 +628,8 @@ void NGPaintFragment::MarkLineBoxesDirtyFor(const LayoutObject& layout_object) {
   }
 }
 
-void NGPaintFragment::MarkLineBoxDirty() {
+void NGPaintFragment::MarkContainingLineBoxDirty() {
+  DCHECK(PhysicalFragment().IsInline() || PhysicalFragment().IsLineBox());
   for (NGPaintFragment* fragment :
        NGPaintFragmentTraversal::InclusiveAncestorsOf(*this)) {
     if (fragment->is_dirty_inline_)
@@ -647,7 +648,7 @@ bool NGPaintFragment::TryMarkFirstLineBoxDirtyFor(
   // Once we reuse lines below dirty lines, we should mark lines for all
   // inline fragments.
   if (NGPaintFragment* const fragment = layout_object.FirstInlineFragment()) {
-    fragment->MarkLineBoxDirty();
+    fragment->MarkContainingLineBoxDirty();
     return true;
   }
   return false;
@@ -660,7 +661,7 @@ bool NGPaintFragment::TryMarkLastLineBoxDirtyFor(
   // Once we reuse lines below dirty lines, we should mark lines for all
   // inline fragments.
   if (NGPaintFragment* const fragment = layout_object.FirstInlineFragment()) {
-    fragment->LastForSameLayoutObject()->MarkLineBoxDirty();
+    fragment->LastForSameLayoutObject()->MarkContainingLineBoxDirty();
     return true;
   }
   return false;

third_party/blink/renderer/core/paint/ng/ng_paint_fragment.h

@@ -279,7 +279,13 @@ class CORE_EXPORT NGPaintFragment : public RefCounted<NGPaintFragment>,
 
   // Mark this line box was changed, in order to re-use part of an inline
   // formatting context.
-  void MarkLineBoxDirty();
+  void MarkLineBoxDirty() {
+    DCHECK(PhysicalFragment().IsLineBox());
+    is_dirty_inline_ = true;
+  }
+
+  // Mark the line box that contains this fragment dirty.
+  void MarkContainingLineBoxDirty();
 
   // Computes LocalVisualRect for an inline LayoutObject in the
   // LayoutObject::LocalVisualRect semantics; i.e., physical coordinates with