Bug 56909 – Add simplified layout path optimization for overflow recomputation and for positioned objects inside relative positioned containers.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dave Hyatt]

Add simplified layout path optimization for overflow recomputation and for positioned objects inside relative positioned containers.

Currently there is an optimized code path for positioned objects, but as soon as we encountered a normal flow object in the containing block chain, we lost the optimization effectively.  This results in the IE MazeSolver page being way slower than it needs to be.

We can enhance the optimization to propagate the fact that we want a simplified layout that just recomputes overflow and lays out those positioned objects without doing any other work (like clearing the expensive-to-rebuild floating objects lists).

[Dave Hyatt]

Created attachment 86590 [details]
Patch

[WebKit Review Bot]

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

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

Source/WebCore/rendering/style/RenderStyle.cpp:500:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 12 files


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

[Dave Hyatt]

Comment on attachment 86590 [details]
Patch

Clearing flags, since there are some things I want to clean up first.

[Dave Hyatt]

Created attachment 86599 [details]
Patch

[WebKit Review Bot]

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

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

Source/WebCore/rendering/style/RenderStyle.cpp:496:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 1 in 12 files


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

[Dave Hyatt]

Created attachment 86628 [details]
Patch

[WebKit Review Bot]

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

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

Source/WebCore/rendering/style/RenderStyle.cpp:496:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 1 in 12 files


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

[Dave Hyatt]

Created attachment 86662 [details]
Patch updated to ToT

[WebKit Review Bot]

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

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

Source/WebCore/rendering/RenderBox.cpp:293:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 12 files


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

[Dave Hyatt]

Created attachment 86665 [details]
Patch updated to ToT

Comment on attachment 86665 [details]
Patch updated to ToT

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

> Source/WebCore/ChangeLog:16
> +        a transform changes, this is the hint returned now instead of a full layout.

It looks like you are not handling opacity changes in this version of the patch.

> Source/WebCore/rendering/RenderObject.h:-999
> -            if (last->style()->top().isAuto() && last->style()->bottom().isAuto()) {
> -                RenderObject* parent = last->parent();
> -                if (!parent->normalChildNeedsLayout()) {
> -                    parent->setChildNeedsLayout(true, false);
> -                    if (parent != newRoot)
> -                        parent->markContainingBlocksForLayout(scheduleRelayout, newRoot);
> -                }
> -            }

I am concerned about this change. It’s not clear to me how the new code added in RenderBox correctly accounts for the need to mark parents of the statically-positioned object. There were subtle cases when failing to do this cause the layout root to not be set correctly. I know that you pass all tests, but I am still worried that the tests only partially cover this.

Comment on attachment 86665 [details]
Patch updated to ToT

I can’t really come up with a case that is covered by the two-chain marking, so given that existing tests pass, I am going to r+

[Dave Hyatt]

Yeah, as mentioned in IRC, the old code would two-chain mark when removals/additions occurred in the tree.  The old code would also two-chain mark whenever anything changed that resulted in a layout and not just margins.

I think the scope of the new code should protect us from risk.  Other browsers don't even pass the dynamic margin change causing static position to update, so I doubt it's commonly used on the Web. :)

[Dave Hyatt]

Fixed in r81802.

[WebKit Review Bot]

http://trac.webkit.org/changeset/81802 might have broken Leopard Intel Release (Tests)
The following tests are not passing:
animations/state-at-end-event.html

[Dave Hyatt]

Rolling this out for now.  There are complications with staticX that get more obvious when you try to extend the optimization.  Attaching a test case that illustrates a failure even without this patch.

[Dave Hyatt]

Created attachment 86707 [details]
Test case that even fails in ToT

[Dave Hyatt]

Created attachment 86708 [details]
Test case that even fails in ToT

[Dave Hyatt]

Created attachment 86866 [details]
Patch

[WebKit Review Bot]

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

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

Source/WebCore/rendering/RenderBlockLineLayout.cpp:2091:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 59 files


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

[Dave Hyatt]

Created attachment 86963 [details]
Patch that consists only of the delta from the last r+ patch

[Dave Hyatt]

I put up an additional patch that shows the additions since the last r+.  Hopefully that makes the main patch easier to review.

Comment on attachment 86866 [details]
Patch

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

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:879
> +                for (unsigned i = 0; i < positionedObjects.size(); ++i)

size() returns a size_t so we typically use a size_t for the index variable

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2098
> +            for (unsigned i = 0; i < trailingPositionedBoxes.size(); ++i) {

same comment about size_t

[Dave Hyatt]

Landed in r81992.

[Yuta Kitamura]

Hi,

I think this patch is causing a lot of crashes in Chromium.

(More accurately, there is a suspicious revision in r81992:r81997; I think it is this patch, but I cannot confirm right now because I don't have build environment at hand.)

Crash trace looks like:
http://build.chromium.org/p/chromium.webkit/builders/Win%20Reliability/builds/1200/steps/reliability%3A%20partial%20result%20of%20current%20build/logs/stdio

Do you have any idea about this?

[Dave Hyatt]

(In reply to comment #25)
> Hi,
> 
> I think this patch is causing a lot of crashes in Chromium.
> 
> (More accurately, there is a suspicious revision in r81992:r81997; I think it is this patch, but I cannot confirm right now because I don't have build environment at hand.)
> 
> Crash trace looks like:
> http://build.chromium.org/p/chromium.webkit/builders/Win%20Reliability/builds/1200/steps/reliability%3A%20partial%20result%20of%20current%20build/logs/stdio
> 
> Do you have any idea about this?

Will look into it.

[Dave Hyatt]

Filed https://bugs.webkit.org/show_bug.cgi?id=57217 to cover the crash reports.

[Simon Fraser (smfr)]

This caused bug 61174.