Bug 56909 - Add simplified layout path optimization for overflow recomputation and for positioned objects inside relative positioned containers.
: Add simplified layout path optimization for overflow recomputation and for po...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To: Dave Hyatt
:
Depends on: 56963
Blocks: 58176 58500
  Show dependency treegraph
 
Reported: 2011-03-23 02:18 PDT by Dave Hyatt
Modified: 2011-05-20 08:09 PDT (History)
6 users (show)

See Also:


Attachments
Patch (20.95 KB, patch)
2011-03-23 02:24 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (22.77 KB, patch)
2011-03-23 03:55 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (22.61 KB, patch)
2011-03-23 09:31 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch updated to ToT (22.68 KB, patch)
2011-03-23 12:14 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch updated to ToT (22.68 KB, patch)
2011-03-23 12:20 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Test case that even fails in ToT (494 bytes, text/html)
2011-03-23 15:22 PDT, Dave Hyatt
no flags Details
Test case that even fails in ToT (597 bytes, text/html)
2011-03-23 15:23 PDT, Dave Hyatt
no flags Details
Patch (1.18 MB, patch)
2011-03-24 18:38 PDT, Dave Hyatt
mitz: review+
Details | Formatted Diff | Diff
Patch that consists only of the delta from the last r+ patch (20.20 KB, patch)
2011-03-25 11:32 PDT, Dave Hyatt
no flags 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-23 02:18:25 PDT
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).
Comment 1 Dave Hyatt 2011-03-23 02:24:32 PDT
Created attachment 86590 [details]
Patch
Comment 2 WebKit Review Bot 2011-03-23 02:26:26 PDT
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.
Comment 3 Dave Hyatt 2011-03-23 03:26:53 PDT
Comment on attachment 86590 [details]
Patch

Clearing flags, since there are some things I want to clean up first.
Comment 4 Dave Hyatt 2011-03-23 03:55:15 PDT
Created attachment 86599 [details]
Patch
Comment 5 WebKit Review Bot 2011-03-23 03:56:47 PDT
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.
Comment 6 Dave Hyatt 2011-03-23 09:31:44 PDT
Created attachment 86628 [details]
Patch
Comment 7 WebKit Review Bot 2011-03-23 09:35:08 PDT
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.
Comment 8 Dave Hyatt 2011-03-23 12:14:52 PDT
Created attachment 86662 [details]
Patch updated to ToT
Comment 9 WebKit Review Bot 2011-03-23 12:16:34 PDT
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.
Comment 10 Dave Hyatt 2011-03-23 12:20:47 PDT
Created attachment 86665 [details]
Patch updated to ToT
Comment 11 mitz@webkit.org 2011-03-23 12:53:01 PDT
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 12 mitz@webkit.org 2011-03-23 13:21:21 PDT
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+
Comment 13 Dave Hyatt 2011-03-23 13:26:48 PDT
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. :)
Comment 14 Dave Hyatt 2011-03-23 13:40:20 PDT
Fixed in r81802.
Comment 15 WebKit Review Bot 2011-03-23 14:41:51 PDT
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
Comment 16 Dave Hyatt 2011-03-23 15:21:34 PDT
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.
Comment 17 Dave Hyatt 2011-03-23 15:22:16 PDT
Created attachment 86707 [details]
Test case that even fails in ToT
Comment 18 Dave Hyatt 2011-03-23 15:23:46 PDT
Created attachment 86708 [details]
Test case that even fails in ToT
Comment 19 Dave Hyatt 2011-03-24 18:38:13 PDT
Created attachment 86866 [details]
Patch
Comment 20 WebKit Review Bot 2011-03-24 18:42:11 PDT
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.
Comment 21 Dave Hyatt 2011-03-25 11:32:43 PDT
Created attachment 86963 [details]
Patch that consists only of the delta from the last r+ patch
Comment 22 Dave Hyatt 2011-03-25 11:35:03 PDT
I put up an additional patch that shows the additions since the last r+.  Hopefully that makes the main patch easier to review.
Comment 23 mitz@webkit.org 2011-03-25 13:46:50 PDT
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
Comment 24 Dave Hyatt 2011-03-25 14:57:38 PDT
Landed in r81992.
Comment 25 Yuta Kitamura 2011-03-27 10:36:03 PDT
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?
Comment 26 Dave Hyatt 2011-03-28 02:01:43 PDT
(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.
Comment 27 Dave Hyatt 2011-03-28 02:03:03 PDT
Filed https://bugs.webkit.org/show_bug.cgi?id=57217 to cover the crash reports.
Comment 28 Simon Fraser (smfr) 2011-05-20 08:09:10 PDT
This caused bug 61174.