Bug 57565 – Optimize line layout to avoid overflow computation and allocation

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Dave Hyatt]

Optimize line layout to avoid overflow computation and allocation.  RenderOverflows are allocated in quirks mode when they don't need to be.  We also do a whole crawl of the line to compute overflow when none is going to exist.

[Dave Hyatt]

Created attachment 87772 [details]
Patch

[WebKit Review Bot]

Attachment 87772 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/InlineFlowBox.cpp:744:  Extra space for operator !   [whitespace/operators] [4]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 87772 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87772&action=review

> Source/WebCore/ChangeLog:8
> +        Optimize overflow computations on lines to avoid allocating RenderOverflows in nearly all cases and to avoid even having
> +        to check the line for overflow in the first place.

Would be nice to quantify the gain here.

[Darin Adler]

Comment on attachment 87772 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87772&action=review

I suspect you could get even more speedup by tuning the inlining so that the rare case was out of line more often.

> Source/WebCore/rendering/EllipsisBox.cpp:32
> -void EllipsisBox::paint(PaintInfo& paintInfo, int tx, int ty)
> +void EllipsisBox::paint(PaintInfo& paintInfo, int tx, int ty, int lineTop, int lineBottom)

As these get more and more arguments I wonder if we might want to use a struct in the future.

> Source/WebCore/rendering/InlineBox.cpp:162
> +void InlineBox::paint(PaintInfo& paintInfo, int tx, int ty, int /* lineTop */, int /*lineBottom*/)

I think you could just leave these names out instead of including comments.

> Source/WebCore/rendering/InlineBox.cpp:290
> +    m_knownToHaveNoOverflow = false;
> +    if (parent() && parent()->knownToHaveNoOverflow())
> +        parent()->clearKnownToHaveNoOverflow();

This might be better as a loop instead of recursion.

> Source/WebCore/rendering/InlineFlowBox.cpp:153
> +        if (child->isText()) {
> +            RenderStyle* childStyle = child->renderer()->style(m_firstLine);
> +            if (childStyle->letterSpacing() < 0 || childStyle->textShadow() || childStyle->textEmphasisMark() != TextEmphasisMarkNone || childStyle->textStrokeWidth())
> +                child->clearKnownToHaveNoOverflow();
> +        } else if (child->renderer()->isReplaced()) {
> +            RenderBox* box = toRenderBox(child->renderer());
> +            if (box->hasRenderOverflow() || box->hasSelfPaintingLayer())
> +                child->clearKnownToHaveNoOverflow();
> +        } else if (!child->renderer()->isBR() && (child->renderer()->style(m_firstLine)->boxShadow() || child->boxModelObject()->hasSelfPaintingLayer()
> +                   || (child->renderer()->isListMarker() && !toRenderListMarker(child->renderer())->isInside())))
> +            child->clearKnownToHaveNoOverflow();

Why not just skip all this work if child->knownToHaveNoOverflow is already false? You are doing that elsewhere.

> Source/WebCore/rendering/InlineFlowBox.cpp:854
> -    IntRect frameBox = enclosingIntRect(FloatRect(x(), y(), width(), height()));
> -    if (frameBox == rect || rect.isEmpty())
> +    IntRect frameBox = enclosingIntRect(frameRectIncludingLineHeight(lineTop, lineBottom));
> +    if (frameBox.contains(rect) || rect.isEmpty())
>          return;

Maybe the rect.isEmpty() check should come first. Why compute frameBox in that case?

> Source/WebCore/rendering/InlineFlowBox.cpp:866
> -    IntRect frameBox = enclosingIntRect(FloatRect(x(), y(), width(), height()));
> -    if (frameBox == rect || rect.isEmpty())
> +    IntRect frameBox = enclosingIntRect(frameRectIncludingLineHeight(lineTop, lineBottom));
> +    if (frameBox.contains(rect) || rect.isEmpty())
>          return;

Maybe the rect.isEmpty() check should come first. Why compute frameBox in that case?

> Source/WebCore/rendering/RenderBox.h:401
> +    RenderOverflow* hasRenderOverflow() const { return m_overflow.get(); }

This is named “has” but it is not a boolean. You should change the name or the return type.

[Dave Hyatt]

Fixed in r82611.

[WebKit Review Bot]

http://trac.webkit.org/changeset/82611 might have broken Qt Linux Release
The following tests are not passing:
fast/multicol/layers-in-multicol.html

This broke repaint (repaint rect is offset horizontally) in the following case:


<style>
    span:hover { color: blue; }
</style>
<div style="width: 250px;">
    Lorem <span>ipsum<br>dolor sit amet</span>
</div>

Filed bug 57735.

[Adam Barth]

We show this change as a ~4.5% speedup in the moz page cycler:

http://build.chromium.org/f/chromium/perf/xp-release-dual-core/moz/report.html?history=150&rev=-1

[Marc-Antoine Ruel]

Comment on attachment 87772 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87772&action=review

> Source/WebCore/rendering/InlineFlowBox.h:198
> +    int logicalLeftLayoutOverflow() const { return m_overflow ? (isHorizontal() ? m_overflow->minXLayoutOverflow() : m_overflow->minYLayoutOverflow()) : logicalLeft(); }

This generates on VS2008:
warning C4244: 'return' : conversion from 'float' to 'int', possible loss of data

It happens because logicalLeft() returns a float. A static_cast<int>() should be used to clarify the intent.

Lines 200, 201, 226, 227, 255, 256, 261.
I'm at r84919 so the lines may be offset.