Bug 57565 - Optimize line layout to avoid overflow computation and allocation
Summary: Optimize line layout to avoid overflow computation and allocation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-31 11:13 PDT by Dave Hyatt
Modified: 2011-04-26 13:17 PDT (History)
5 users (show)

See Also:


Attachments
Patch (231.10 KB, patch)
2011-03-31 11:21 PDT, Dave Hyatt
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2011-03-31 11:13:43 PDT
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.
Comment 1 Dave Hyatt 2011-03-31 11:21:51 PDT
Created attachment 87772 [details]
Patch
Comment 2 WebKit Review Bot 2011-03-31 11:26:04 PDT
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.
Comment 3 Simon Fraser (smfr) 2011-03-31 11:29:46 PDT
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.
Comment 4 Darin Adler 2011-03-31 11:32:33 PDT
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.
Comment 5 Dave Hyatt 2011-03-31 13:41:58 PDT
Fixed in r82611.
Comment 6 WebKit Review Bot 2011-03-31 14:26:13 PDT
http://trac.webkit.org/changeset/82611 might have broken Qt Linux Release
The following tests are not passing:
fast/multicol/layers-in-multicol.html
Comment 7 mitz 2011-04-03 15:08:16 PDT
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>
Comment 8 mitz 2011-04-03 15:11:32 PDT
Filed bug 57735.
Comment 9 Adam Barth 2011-04-03 18:39:46 PDT
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
Comment 10 Marc-Antoine Ruel 2011-04-26 13:17:14 PDT
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.