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
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
: 56963
: 58176 58500
  Show dependency treegraph
 
Reported: 2011-03-23 02:18 PST by
Modified: 2011-05-20 08:09 PST (History)


Attachments
Patch (20.95 KB, patch)
2011-03-23 02:24 PST, Dave Hyatt
no flags Review Patch | Details | Formatted Diff | Diff
Patch (22.77 KB, patch)
2011-03-23 03:55 PST, Dave Hyatt
no flags Review Patch | Details | Formatted Diff | Diff
Patch (22.61 KB, patch)
2011-03-23 09:31 PST, Dave Hyatt
no flags Review Patch | Details | Formatted Diff | Diff
Patch updated to ToT (22.68 KB, patch)
2011-03-23 12:14 PST, Dave Hyatt
no flags Review Patch | Details | Formatted Diff | Diff
Patch updated to ToT (22.68 KB, patch)
2011-03-23 12:20 PST, Dave Hyatt
no flags Review Patch | Details | Formatted Diff | Diff
Test case that even fails in ToT (494 bytes, text/html)
2011-03-23 15:22 PST, Dave Hyatt
no flags Details
Test case that even fails in ToT (597 bytes, text/html)
2011-03-23 15:23 PST, Dave Hyatt
no flags Details
Patch (1.18 MB, patch)
2011-03-24 18:38 PST, Dave Hyatt
mitz: review+
Review Patch | 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 PST, Dave Hyatt
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


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