Commit 1b4bf96b authored by Koji Ishii's avatar Koji Ishii Committed by Commit Bot

Remove |IsPartOfCulledInlineBox| for hit-testing culled inlines

This patch reverts the heuristic logic for hit-testing culled
inlines added in r807114 <crrev.com/c/1127976>. This change
makes hit-testing of a small number of really long lines (such
as minimized HTML source) faster. The test in crbug.com/128937
improves from 180ms to 40ms.

Reverting manually because r807114 has a test, which we don't
want to revert.

Summarizing the history of changes around hit-test performance
of culled inlines:
1. Initially, NG took a different approach than legacy, to
   traverse fragments and check |LayoutObject| ancestors to
   find culled inlines. Legacy traverses |LayoutObject| tree.
2. r793989 crrev.com/c/2332083 changed to traverse
   |LayoutObject| to fix |getBoundingClientRect| performance.
3. r807114 crrev.com/c/1127976 reverted it only for descendant
   cursors. Hit-testing uses this codepath.
   This improved the performance for a large number of lines,
   but sacrificed a small number of really long lines.
4. r815524 crrev.com/c/2463086 added an early return to
   |NGBoxFragmentPainter::HitTestLineBoxFragment|.
   This improves NG more than the 3rd fix did, producing the
   similar performance as legacy both for a large number of
   lines and for a small number of lines.

The 3rd fix still improves when there are many lines. This
revert reduces `blink_perf/layout/culled-inline-hittest` from
3ms to 4ms, but:
* The 4th fix made it fast enough to not need the improvement.
* The sacrifice for a small number of lines is high.
* The maintainance cost for |IsPartOfCulledInlineBox| is high.
hence reverting the 3rd fix.

If this revert causes issues in some other cases, we could
bring this logic back in, with stricter conditions (only for a
large number of lines). But the numbers look good enough and
legacy does not have this heuristic that I don't think this
will be needed.

Bug: 1128937, 1127976
Change-Id: Ib411cf43727d2ba34a935fde98139a3f34261df6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2463026Reviewed-by: default avatarYoshifumi Inoue <yosin@chromium.org>
Reviewed-by: default avatarIan Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815948}
parent 0c4c1d89
...@@ -269,32 +269,6 @@ bool NGInlineCursorPosition::IsInlineLeaf() const { ...@@ -269,32 +269,6 @@ bool NGInlineCursorPosition::IsInlineLeaf() const {
return !IsListMarker(); return !IsListMarker();
} }
bool NGInlineCursorPosition::IsPartOfCulledInlineBox(
const LayoutInline& layout_inline) const {
DCHECK(!layout_inline.ShouldCreateBoxFragment());
DCHECK(*this);
const LayoutObject* const layout_object = GetLayoutObject();
// We use |IsInline()| to exclude floating and out-of-flow objects.
if (!layout_object || !layout_object->IsInline() ||
layout_object->IsAtomicInlineLevel())
return false;
DCHECK(!layout_object->IsFloatingOrOutOfFlowPositioned());
DCHECK(!BoxFragment() || !BoxFragment()->IsFormattingContextRoot());
for (const LayoutObject* parent = layout_object->Parent(); parent;
parent = parent->Parent()) {
// Children of culled inline should be included.
if (parent == &layout_inline)
return true;
// Grand children should be included only if children are also culled.
if (const auto* parent_layout_inline = ToLayoutInlineOrNull(parent)) {
if (!parent_layout_inline->ShouldCreateBoxFragment())
continue;
}
return false;
}
return false;
}
bool NGInlineCursor::IsLastLineInInlineBlock() const { bool NGInlineCursor::IsLastLineInInlineBlock() const {
DCHECK(Current().IsLineBox()); DCHECK(Current().IsLineBox());
if (!GetLayoutBlockFlow()->IsAtomicInlineLevel()) if (!GetLayoutBlockFlow()->IsAtomicInlineLevel())
...@@ -1700,23 +1674,15 @@ const LayoutObject* NGInlineCursor::CulledInlineTraversal::Find( ...@@ -1700,23 +1674,15 @@ const LayoutObject* NGInlineCursor::CulledInlineTraversal::Find(
return nullptr; return nullptr;
} }
void NGInlineCursor::CulledInlineTraversal::SetUseFragmentTree(
const LayoutInline& layout_inline) {
layout_inline_ = &layout_inline;
use_fragment_tree_ = true;
}
const LayoutObject* NGInlineCursor::CulledInlineTraversal::MoveToFirstFor( const LayoutObject* NGInlineCursor::CulledInlineTraversal::MoveToFirstFor(
const LayoutInline& layout_inline) { const LayoutInline& layout_inline) {
layout_inline_ = &layout_inline; layout_inline_ = &layout_inline;
use_fragment_tree_ = false;
current_object_ = Find(layout_inline.FirstChild()); current_object_ = Find(layout_inline.FirstChild());
return current_object_; return current_object_;
} }
const LayoutObject* NGInlineCursor::CulledInlineTraversal::MoveToNext() { const LayoutObject* NGInlineCursor::CulledInlineTraversal::MoveToNext() {
if (!current_object_) DCHECK(current_object_);
return nullptr;
current_object_ = current_object_ =
Find(current_object_->NextInPreOrderAfterChildren(layout_inline_)); Find(current_object_->NextInPreOrderAfterChildren(layout_inline_));
return current_object_; return current_object_;
...@@ -1724,19 +1690,6 @@ const LayoutObject* NGInlineCursor::CulledInlineTraversal::MoveToNext() { ...@@ -1724,19 +1690,6 @@ const LayoutObject* NGInlineCursor::CulledInlineTraversal::MoveToNext() {
void NGInlineCursor::MoveToFirstForCulledInline( void NGInlineCursor::MoveToFirstForCulledInline(
const LayoutInline& layout_inline) { const LayoutInline& layout_inline) {
// When |this| is a descendant cursor, |this| may be limited to a very small
// subset of the |LayoutObject| descendants, and that traversing
// |LayoutObject| descendants is much more expensive. Prefer checking every
// fragment in that case.
if (IsDescendantsCursor()) {
culled_inline_.SetUseFragmentTree(layout_inline);
DCHECK(!CanMoveAcrossFragmentainer());
MoveToFirst();
while (Current() && !Current().IsPartOfCulledInlineBox(layout_inline))
MoveToNext();
return;
}
if (const LayoutObject* layout_object = if (const LayoutObject* layout_object =
culled_inline_.MoveToFirstFor(layout_inline)) { culled_inline_.MoveToFirstFor(layout_inline)) {
MoveTo(*layout_object); MoveTo(*layout_object);
...@@ -1748,16 +1701,6 @@ void NGInlineCursor::MoveToFirstForCulledInline( ...@@ -1748,16 +1701,6 @@ void NGInlineCursor::MoveToFirstForCulledInline(
void NGInlineCursor::MoveToNextForCulledInline() { void NGInlineCursor::MoveToNextForCulledInline() {
DCHECK(culled_inline_); DCHECK(culled_inline_);
if (culled_inline_.UseFragmentTree()) {
const LayoutInline* layout_inline = culled_inline_.GetLayoutInline();
DCHECK(layout_inline);
DCHECK(!CanMoveAcrossFragmentainer());
do {
MoveToNext();
} while (Current() && !Current().IsPartOfCulledInlineBox(*layout_inline));
return;
}
MoveToNextForSameLayoutObjectExceptCulledInline(); MoveToNextForSameLayoutObjectExceptCulledInline();
// If we're at the end of fragments for the current |LayoutObject| that // If we're at the end of fragments for the current |LayoutObject| that
// contributes to the current culled inline, find the next |LayoutObject|. // contributes to the current culled inline, find the next |LayoutObject|.
......
...@@ -532,13 +532,8 @@ class CORE_EXPORT NGInlineCursor { ...@@ -532,13 +532,8 @@ class CORE_EXPORT NGInlineCursor {
public: public:
CulledInlineTraversal() = default; CulledInlineTraversal() = default;
const LayoutInline* GetLayoutInline() const { return layout_inline_; } explicit operator bool() const { return current_object_; }
void Reset() { current_object_ = nullptr; }
explicit operator bool() const { return layout_inline_; }
void Reset() { layout_inline_ = nullptr; }
bool UseFragmentTree() const { return use_fragment_tree_; }
void SetUseFragmentTree(const LayoutInline& layout_inline);
// Returns first/next |LayoutObject| that contribute to |layout_inline|. // Returns first/next |LayoutObject| that contribute to |layout_inline|.
const LayoutObject* MoveToFirstFor(const LayoutInline& layout_inline); const LayoutObject* MoveToFirstFor(const LayoutInline& layout_inline);
...@@ -549,7 +544,6 @@ class CORE_EXPORT NGInlineCursor { ...@@ -549,7 +544,6 @@ class CORE_EXPORT NGInlineCursor {
const LayoutObject* current_object_ = nullptr; const LayoutObject* current_object_ = nullptr;
const LayoutInline* layout_inline_ = nullptr; const LayoutInline* layout_inline_ = nullptr;
bool use_fragment_tree_ = false;
}; };
void MoveToFirstForCulledInline(const LayoutInline& layout_inline); void MoveToFirstForCulledInline(const LayoutInline& layout_inline);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment