Bug 95053 – Enable SATURATED_LAYOUT_ARITHMETIC for chromium

• Component: Layout and Rendering
• Status: RESOLVED WONTFIX

[Emil A Eklund]

Enable SATURATED_LAYOUT_ARITHMETIC for the chromium port.

[Emil A Eklund]

Created attachment 160653 [details]
Patch

[Emil A Eklund]

There are a couple of tests that rely on overflow that I'll likely have to rebaseline, waiting for the crbot to see what it thinks.

[WebKit Review Bot]

Comment on attachment 160653 [details]
Patch

Attachment 160653 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13619039

New failing tests:
fast/block/float/overhanging-tall-block.html
css3/flexbox/flex-algorithm.html

[WebKit Review Bot]

Created attachment 160673 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Abhishek Arya]

ccing Ojan and Tony since there is a flexbox overflow case here.

--- /tmp/layout-test-results/css3/flexbox/flex-algorithm-expected.txt 
+++ /tmp/layout-test-results/css3/flexbox/flex-algorithm-actual.txt 
@@ -12,7 +12,14 @@
 PASS
 PASS
 PASS
-PASS
+FAIL:
+Expected 0 for width, but got -35792593. 
+
+<div class="flexbox">
+  <div data-expected-width="600" style="-webkit-flex: 100000000000000000000000000000000000000 0 600px"></div>
+  <div data-expected-width="600" style="-webkit-flex: 0 100000000000000000000000000000000000000 600px"></div>
+  <div data-expected-width="0" style="-webkit-flex: 1 1 100000000000000000000000000000000000000px"></div>
+</div>
 PASS
 PASS
 PASS

[Tony Chang]

I'm not sure why that's failing.  Specifically, it's the third flex item with a flex basis of 100..00px that's failing.

In the flexbox code, we just pass the value on as a Length.  Look for callers of RenderFlexibleBox::flexBasisForChild.

[Emil A Eklund]

(In reply to comment #6)
> I'm not sure why that's failing.  Specifically, it's the third flex item with a flex basis of 100..00px that's failing.
> 
> In the flexbox code, we just pass the value on as a Length.  Look for callers of RenderFlexibleBox::flexBasisForChild.

I'll dig into it and let you know if I need any help. Thanks.

[Emil A Eklund]

(In reply to comment #6)
> I'm not sure why that's failing.  Specifically, it's the third flex item with a flex basis of 100..00px that's failing.
> 
> In the flexbox code, we just pass the value on as a Length.  Look for callers of RenderFlexibleBox::flexBasisForChild.

It actually looks like the test in question relies on the value overflowing. 
100000000000000000000000000000000000000 is represented as 
 99999996802856924650656260769173209088 (float) and when this is converted to a layout unit it overflows and results in the value -35791396 (layout unit). This value is then clamped to 0 in preferredMainAxisContentExtentForChild using std::max.

With SATURATED_LAYOUT_ARITHMETIC enabled 99999996802856924650656260769173209088 translates to the 35791396 instead, which is the maximum allowed value for LayoutUnit.

[Emil A Eklund]

FYI: Bug 95883 deals with the issues that causes the flex test to fail.

[Emil A Eklund]

Committed r129982: <http://trac.webkit.org/changeset/129982>

[Eric Seidel (no email)]

Yay!

[Ojan Vafai]

This patch hits an assert running fast/overflow/overflow-height-float-not-removed-crash3.html on Chromium Linux and Mac Debug.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Foverflow%2Foverflow-height-float-not-removed-crash3.html

STDERR: ASSERTION FAILED: oldLayoutDelta == view()->layoutDelta()
STDERR: third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp(2577) : void WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
STDERR: [17268:17268:16408045412109:ERROR:process_util_posix.cc(144)] Received signal 11
STDERR: 	base::debug::StackTrace::StackTrace() [0x7fa7aad6ec6a]
STDERR: 	base::(anonymous namespace)::StackDumpSignalHandler() [0x7fa7aadd7d59]
STDERR: 	0x7fa7a4023af0
STDERR: 	WebCore::RenderBlock::layoutBlockChild() [0x7fa7ae8d37de]
STDERR: 	WebCore::RenderBlock::layoutBlockChildren() [0x7fa7ae8d2b79]
STDERR: 	WebCore::RenderBlock::layoutBlock() [0x7fa7ae8cddbe]
STDERR: 	WebCore::RenderBlock::layout() [0x7fa7ae8cd26e]
STDERR: 	WebCore::RenderObject::layoutIfNeeded() [0x7fa7ae89e727]
STDERR: 	WebCore::RenderBlock::layoutInlineChildren() [0x7fa7ae91d45a]
STDERR: 	WebCore::RenderBlock::layoutBlock() [0x7fa7ae8cdd9d]
STDERR: 	WebCore::RenderBlock::layout() [0x7fa7ae8cd26e]
STDERR: 	WebCore::RenderBlock::layoutBlockChild() [0x7fa7ae8d3002]
STDERR: 	WebCore::RenderBlock::layoutBlockChildren() [0x7fa7ae8d2b79]
STDERR: 	WebCore::RenderBlock::layoutBlock() [0x7fa7ae8cddbe]
STDERR: 	WebCore::RenderBlock::layout() [0x7fa7ae8cd26e]
STDERR: 	WebCore::RenderObject::layoutIfNeeded() [0x7fa7ae89e727]
STDERR: 	WebCore::RenderBlock::insertFloatingObject() [0x7fa7ae8dc0b8]
STDERR: 	WebCore::RenderBlock::LineBreaker::skipLeadingWhitespace() [0x7fa7ae91f8fc]
STDERR: 	WebCore::RenderBlock::LineBreaker::nextLineBreak() [0x7fa7ae92084a]
STDERR: 	WebCore::RenderBlock::layoutRunsAndFloatsInRange() [0x7fa7ae91b5b8]
STDERR: 	WebCore::RenderBlock::layoutRunsAndFloats() [0x7fa7ae91ae6e]
STDERR: 	WebCore::RenderBlock::layoutInlineChildren() [0x7fa7ae91d49c]
STDERR: 	WebCore::RenderBlock::layoutBlock() [0x7fa7ae8cdd9d]
STDERR: 	WebCore::RenderBlock::layout() [0x7fa7ae8cd26e]
STDERR: 	WebCore::RenderBlock::layoutBlockChild() [0x7fa7ae8d3002]
STDERR: 	WebCore::RenderBlock::layoutBlockChildren() [0x7fa7ae8d2b79]
STDERR: 	WebCore::RenderBlock::layoutBlock() [0x7fa7ae8cddbe]
STDERR: 	WebCore::RenderBlock::layout() [0x7fa7ae8cd26e]
STDERR: 	WebCore::RenderBlock::layoutBlockChild() [0x7fa7ae8d3002]
STDERR: 	WebCore::RenderBlock::layoutBlockChildren() [0x7fa7ae8d2b79]
STDERR: 	WebCore::RenderBlock::layoutBlock() [0x7fa7ae8cddbe]
STDERR: 	WebCore::RenderBlock::layout() [0x7fa7ae8cd26e]
STDERR: 	WebCore::RenderBlock::layoutBlockChild() [0x7fa7ae8d3002]
STDERR: 	WebCore::RenderBlock::layoutBlockChildren() [0x7fa7ae8d2b79]
STDERR: 	WebCore::RenderBlock::layoutBlock() [0x7fa7ae8cddbe]
STDERR: 	WebCore::RenderBlock::layout() [0x7fa7ae8cd26e]
STDERR: 	WebCore::RenderView::layout() [0x7fa7aea872be]
STDERR: 	WebCore::FrameView::layout() [0x7fa7ae797db2]
STDERR: 	WebCore::FrameView::layoutTimerFired() [0x7fa7ae79b703]

[Emil A Eklund]

Yeah, saw that and am looking into it.

[WebKit Review Bot]

Re-opened since this is blocked by bug 97971

[Emil A Eklund]

Rolling out for now.

We currently overflow/wrap when computing the delta in RenderBlock::setLogicalTopForChild for that test (given the massive height of the text areas) when the delta is later added back in RenderBlock::layoutBlockChild the number wraps again getting us back to the correct value.

With SATURATED_LAYOUT_ARITHMETIC enabled the values no longer wraps, which seems like the correct thing to do however this causes the compare to fail for obvious reasons. Rolling out the flag flip until we can figure out the best way to handle this case. We might have to special case situations where the delta is saturated.

[Emil A Eklund]

Marking as reopened.

[Eric Seidel (no email)]

Do we have perf numbers for saturated arithmetic?  I'm sure we must have discussed such at some point.

[Emil A Eklund]

(In reply to comment #17)
> Do we have perf numbers for saturated arithmetic?  I'm sure we must have discussed such at some point.

The only way to get accurate metrics is to run it through the PLT. We turned it on for about a day late last year to collect PLT data and one of the cyclers showed about a 2% regression if I recall correctly. We've made a bunch of performance improvements since so hopefully the perf impact is significantly smaller this time around.

[Benjamin Poulain]

(In reply to comment #18)
> (In reply to comment #17)
> > Do we have perf numbers for saturated arithmetic?  I'm sure we must have discussed such at some point.
> 
> The only way to get accurate metrics is to run it through the PLT. We turned it on for about a day late last year to collect PLT data and one of the cyclers showed about a 2% regression if I recall correctly. We've made a bunch of performance improvements since so hopefully the perf impact is significantly smaller this time around.

From things I have seen here and there, I think there are still plenty of opportunities for optimizations.

Could someone make a synthetic benchmark focused on performance issues caused by the saturated arithmetic? It would be interesting to improve it further.

[Emil A Eklund]

(In reply to comment #19)
> From things I have seen here and there, I think there are still plenty of opportunities for optimizations.

There is definitely a lot more we can do to optimize it if performance suffers. 
Using the "cmovo" instruction for x86 and "qadd", "qsub" etc for arm we should be able to remove all the branching from the saturated addition, subtraction and multiplication methods. That would likely require platform specific assembly code though. I haven't found a way to write C code that would generate the desired set of instructions yet.

Easier still, in a lot of places we do clamping which we should be able to optimize without too much trouble.

> Could someone make a synthetic benchmark focused on performance issues caused by the saturated arithmetic? It would be interesting to improve it further.

I'll see what I can come up with.

[Benjamin Poulain]

(In reply to comment #20)
> Using the "cmovo" instruction for x86 and "qadd", "qsub" etc for arm we should be able to remove all the branching from the saturated addition, subtraction and multiplication methods. That would likely require platform specific assembly code though. I haven't found a way to write C code that would generate the desired set of instructions yet.

I had similar issues a while ago and like you I could not get the compiler to generate what I wanted from C code. :(

I would be happy to help with ARM assembly (or reviewing it).


> > Could someone make a synthetic benchmark focused on performance issues caused by the saturated arithmetic? It would be interesting to improve it further.
> 
> I'll see what I can come up with.

Awesome. Thank for looking into that.

[Emil A Eklund]

(In reply to comment #21)
> I had similar issues a while ago and like you I could not get the compiler to generate what I wanted from C code. :(
> 
> I would be happy to help with ARM assembly (or reviewing it).

Thanks, I'll likely take you up on that. Don't have much recent experience with ARM assembly.