Bug 56909

Summary: Add simplified layout path optimization for overflow recomputation and for positioned objects inside relative positioned containers.
Product: WebKit Reporter: Dave Hyatt <hyatt>
Component: Layout and RenderingAssignee: Dave Hyatt <hyatt>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, benjamin, eric, simon.fraser, webkit.review.bot, yutak
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 56963    
Bug Blocks: 58176, 58500    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch updated to ToT
none
Patch updated to ToT
none
Test case that even fails in ToT
none
Test case that even fails in ToT
none
Patch
mitz: review+
Patch that consists only of the delta from the last r+ patch none

Dave Hyatt
Reported 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).
Attachments
Patch (20.95 KB, patch)
2011-03-23 02:24 PDT, Dave Hyatt 		no flags
DetailsFormatted DiffDiff
Patch (22.77 KB, patch)
2011-03-23 03:55 PDT, Dave Hyatt 		no flags
DetailsFormatted DiffDiff
Patch (22.61 KB, patch)
2011-03-23 09:31 PDT, Dave Hyatt 		no flags
DetailsFormatted DiffDiff
Patch updated to ToT (22.68 KB, patch)
2011-03-23 12:14 PDT, Dave Hyatt 		no flags
DetailsFormatted DiffDiff
Patch updated to ToT (22.68 KB, patch)
2011-03-23 12:20 PDT, Dave Hyatt 		no flags
DetailsFormatted DiffDiff
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+
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.
Dave Hyatt
Comment 1 2011-03-23 02:24:32 PDT
Created attachment 86590 [details] Patch
WebKit Review Bot
Comment 2 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.
Dave Hyatt
Comment 3 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.
Dave Hyatt
Comment 4 2011-03-23 03:55:15 PDT
Created attachment 86599 [details] Patch
WebKit Review Bot
Comment 5 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.
Dave Hyatt
Comment 6 2011-03-23 09:31:44 PDT
Created attachment 86628 [details] Patch
WebKit Review Bot
Comment 7 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.
Dave Hyatt
Comment 8 2011-03-23 12:14:52 PDT
Created attachment 86662 [details] Patch updated to ToT
WebKit Review Bot
Comment 9 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.
Dave Hyatt
Comment 10 2011-03-23 12:20:47 PDT
Created attachment 86665 [details] Patch updated to ToT
mitz
Comment 11 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.
mitz
Comment 12 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+
Dave Hyatt
Comment 13 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. :)
Dave Hyatt
Comment 14 2011-03-23 13:40:20 PDT
Fixed in r81802.
WebKit Review Bot
Comment 15 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
Dave Hyatt
Comment 16 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.
Dave Hyatt
Comment 17 2011-03-23 15:22:16 PDT
Created attachment 86707 [details] Test case that even fails in ToT
Dave Hyatt
Comment 18 2011-03-23 15:23:46 PDT
Created attachment 86708 [details] Test case that even fails in ToT
Dave Hyatt
Comment 19 2011-03-24 18:38:13 PDT
Created attachment 86866 [details] Patch
WebKit Review Bot
Comment 20 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.
Dave Hyatt
Comment 21 2011-03-25 11:32:43 PDT
Created attachment 86963 [details] Patch that consists only of the delta from the last r+ patch
Dave Hyatt
Comment 22 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.
mitz
Comment 23 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
Dave Hyatt
Comment 24 2011-03-25 14:57:38 PDT
Landed in r81992.
Yuta Kitamura
Comment 25 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?
Dave Hyatt
Comment 26 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.
Dave Hyatt
Comment 27 2011-03-28 02:03:03 PDT
Filed https://bugs.webkit.org/show_bug.cgi?id=57217 to cover the crash reports.
Simon Fraser (smfr)
Comment 28 2011-05-20 08:09:10 PDT
This caused bug 61174.
Note You need to log in before you can comment on or make changes to this bug.