Bug 60318

Created attachment 98021 [details] zooming test case

Created attachment 98022 [details] perf test case

Created attachment 98023 [details] zoom perf test case

Created attachment 98025 [details] Proof of concept I'm attaching a proof of concept patch that's been tested on Mac and QT. This patch changes much, but not all of the rendering tree from integer offsets to floating point.

(In reply to comment #4) > Created an attachment (id=98025) [details] > Proof of concept > > I'm attaching a proof of concept patch that's been tested on Mac and QT. This patch changes much, but not all of the rendering tree from integer offsets to floating point. FYI, this patch was tested with r88496.

Created attachment 100367 [details] Patch switching LayoutUnit int -> float. Do not review. Patch for switching LayoutUnit over to float. It's too early to make the switch but it helps in identifying places we've missed.

Created attachment 139264 [details] Patch

Comment on attachment 139264 [details] Patch Attachment 139264 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12553343

Comment on attachment 139264 [details] Patch Attachment 139264 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12567166

Comment on attachment 139264 [details] Patch Attachment 139264 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12551311

Comment on attachment 139264 [details] Patch Attachment 139264 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12567170

Created attachment 139280 [details] Patch

Comment on attachment 139280 [details] Patch Attachment 139280 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12548366

Created attachment 139296 [details] Patch

Comment on attachment 139296 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139296&action=review > Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:816 > + } while (groupRemainingSpace >= 1); ?

Comment on attachment 139296 [details] Patch Attachment 139296 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12550411

Comment on attachment 139296 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139296&action=review I think a substantial fraction of this patch could be landed separately, first. Even though we’ve carved off so many pieces before, I still think there are quite a few here. > Source/WebCore/css/CSSPrimitiveValue.h:64 > -template<typename T> inline T roundForImpreciseConversion(double value) > -{ > // Dimension calculations are imprecise, often resulting in values of e.g. > // 44.99998. We need to go ahead and round if we're really close to the > // next integer value. > +template<typename T> inline T roundForImpreciseConversion(double value) > +{ > value += (value < 0) ? -0.01 : +0.01; > return ((value > std::numeric_limits<T>::max()) || (value < std::numeric_limits<T>::min())) ? 0 : static_cast<T>(value); > } > > +template<> inline float roundForImpreciseConversion(double value) > +{ > + double ceiledValue = ceil(value); > + double proximityToNextInt = ceiledValue - value; > + if (proximityToNextInt <= 0.01 && value > 0) > + return static_cast<float>(ceiledValue); > + if (proximityToNextInt >= 0.99 && value < 0) > + return static_cast<float>(floor(value)); > + return static_cast<float>(value); > +} > + This change is one example. Also, I don’t understand why the comment is indented now that it's outside a function. > Source/WebCore/page/SpatialNavigation.cpp:648 > - otherAxisDistance = abs(exitPoint.y() - entryPoint.y()); > + otherAxisDistance = (exitPoint.y() - entryPoint.y()).abs(); Can’t we overload the abs function so you can call it rather than having to call a member function? I don’t think we should need to change this at all. > Source/WebCore/platform/FractionalLayoutUnit.h:45 > +#if ENABLE(SUBPIXEL_LAYOUT) > static const int kFixedPointDenominator = 60; > +#else > +static const int kFixedPointDenominator = 1; > +#endif Seems like we could land all these FractionalLayoutUnit.h changes separately, first. > Source/WebCore/platform/graphics/FractionalLayoutRect.cpp:149 > FractionalLayoutRect enclosingFractionalLayoutRect(const FloatRect& rect) > { > +#if ENABLE(SUBPIXEL_LAYOUT) > return FractionalLayoutRect(rect.x(), rect.y(), > rect.maxX() - rect.x() + FractionalLayoutUnit::epsilon(), > rect.maxY() - rect.y() + FractionalLayoutUnit::epsilon()); > +#else > + return enclosingIntRect(rect); > +#endif > } Seems like this could land separately, first. > Source/WebCore/platform/qt/RenderThemeQt.cpp:740 > - IntRect b = toRenderBox(o)->contentBoxRect(); > + IntRect b = pixelSnappedIntRect(toRenderBox(o)->contentBoxRect()); Can’t we land this separately, first? > Source/WebCore/platform/win/PopupMenuWin.cpp:658 > - textX += minimumValueForLength(itemStyle.textIndent(), itemRect.width()); > + textX += minimumIntValueForLength(itemStyle.textIndent(), itemRect.width()); Can’t we land this separately, first? > Source/WebCore/rendering/InlineFlowBox.cpp:597 > - setLogicalTop(top + maxAscent - fontMetrics.ascent(baselineType)); > + setLogicalTop((top + maxAscent - fontMetrics.ascent(baselineType)).round()); Can’t we make round a free function instead of a member function? And overload it for int to do nothing? And then we could land this separately, first. > Source/WebCore/rendering/PaintInfo.h:101 > - static IntRect infiniteRect() { return IntRect(INT_MIN / 2, INT_MIN / 2, INT_MAX, INT_MAX); } > + static IntRect infiniteRect() { return IntRect(LayoutRect::infiniteRect()); } I don’t understand this change. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1760 > - LayoutUnit logicalBottom = lastLine->lineBottomWithLeading() + abs(lineDelta); > + LayoutUnit logicalBottom = lastLine->lineBottomWithLeading() + lineDelta.abs(); Again, if we just overload a function named abs we could do this separately, first. > Source/WebCore/rendering/RenderBoxModelObject.cpp:1240 > - geometry.setPhaseX(geometry.tileSize().width() ? geometry.tileSize().width() - (xPosition + left) % geometry.tileSize().width() : 0); > + geometry.setPhaseX(geometry.tileSize().width() ? geometry.tileSize().width() - lround(xPosition + left) % geometry.tileSize().width() : 0); Can’t we make an overloaded function named round for use here? >> Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:816 >> + } while (groupRemainingSpace >= 1); > > ? Seems like you could land this change separately, first.

Comment on attachment 139296 [details] Patch Attachment 139296 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12550528

Created attachment 139897 [details] Patch

Comment on attachment 139897 [details] Patch I'm concerned about the Release/Debug test differences, but that's really a separate bug. I think we're ready to pull this trigger. I look forward to the real "enable" patch soon. You should glance at the perf bots tomorrow just to make sure you didn't slow down the world. :)

Created attachment 140054 [details] Patch for landing

Created attachment 140063 [details] Patch for landing

(In reply to comment #22) > Created an attachment (id=140063) [details] > Patch for landing (Proactively fixing a build error due to static initializers introduced in r115895)

Committed r116009: <http://trac.webkit.org/changeset/116009>

Comment on attachment 140063 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=140063&action=review > Source/WebCore/rendering/LayoutTypes.h:118 > + return FractionalLayoutUnit(value); As far as I can tell, this doesn't do any rounding, it returns the floor. All the FractionalLayout types needs rounding functions similar to those of the Int types.

There is an additional bug that should be solved while rounding is fixed. In FractionalLayoutUnit the static function epsilon will always return 0 because the division performed is an integer division not a float division. This is relevant because proper rounding needs to add half epsilon to the floating point values before converting them to fixed point values.

Comment on attachment 140063 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=140063&action=review >> Source/WebCore/rendering/LayoutTypes.h:118 >> + return FractionalLayoutUnit(value); > > As far as I can tell, this doesn't do any rounding, it returns the floor. All the FractionalLayout types needs rounding functions similar to those of the Int types. When SUBPIXEL_LAYOUT is enabled it will be floored to the nearest fraction which is sufficiently precise. The lround case below is a stop-gap measure to accommodate the case where SUBPIXEL_LAYOUT is disabled.

It appears that this patch has caused 15-30% performance regressions on various page loading tests: http://webkit-perf.appspot.com/graph.html#tests=[[476025,2001,963028]]&sel=1336044324906.6218,1336124380029.0154,295.1914893617021,416.1276595744681&displayrange=7&datatype=running http://webkit-perf.appspot.com/graph.html#tests=[[472030,2001,3001]]&sel=1336071584445.428,1336135189885.1377,614.2978723404256,724.2127659574468&displayrange=7&datatype=running http://webkit-perf.appspot.com/graph.html#tests=[[363057,2001,963028]]&sel=1336077620237.7356,1336083048782.157,625.8008148483477,815.3807152557719&displayrange=7&datatype=running and several other tests regressed around this changeset.

(In reply to comment #28) > It appears that this patch has caused 15-30% performance regressions on various page loading tests: Given that level of performance regression we need to roll this out as soon as possible. On the other hand, most of what is in this patch can be landed separately (yes, even now after all those previous times we did that sort of thing): SVGRenderTreeAsText.cpp RenderMathMLBlock.cpp RenderTreeAsText.h part or perhaps all of RenderTreeAsText.cpp RenderLayer.h PaintInfo.h (maybe) And perhaps even more. That’s worth doing to reduce the diff even more later when we solve the performance regression problem.

(In reply to comment #29) > (In reply to comment #28) > > It appears that this patch has caused 15-30% performance regressions on various page loading tests: > > Given that level of performance regression we need to roll this out as soon as possible. I'll be cautious about that since page loading tests aren't as robust as I have hoped for it to be. I think we need to at least verify that these regressions are real (e.g. by running the tests locally and comparing results).

I've uploaded a patch that I believe will fix the performance regression on bug 85834. If that fails to adequately address the regression we'll roll out this change and re-land it piece by piece as suggested by Darin.

Performance update So far we haven't seen any performance regressions from this change on any of the page load tests/page cyclers [1] apart from a small regression on the chromium http bloat test [2] that might have been caused by this change. We'll continue to monitor the situation. We are, however, seeing a 5-10% regression - down from slightly higher prior to r116392 - on a perf-o-matic micro-benchmark [3] testing layout of nested floats. We are currently looking into this and will have another update within the next couple of days. 1: http://build.chromium.org/f/chromium/perf/dashboard/overview.html?graph=times 2: http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/bloat-http/report.html?history=300&rev=-1 3: http://webkit-perf.appspot.com/graph.html#tests=[[472030,2001,32196],[472030,2001,173262],[472030,2001,3001]]&sel=1335969265834.5168,1336672079276&displayrange=30&datatype=running

Performance update The potential regression on the http bloat test has been determined not to be an issue. According to the original author it should only be used to measure memory usage. Timing data reported is inaccurate and reportedly irrelevant. As for the nested floats micro-benchmark we have confirmed that it is a real regression caused by this change. However, that benchmark intentionally stresses a code path that is rarely triggered in a real-world scenario and as such is of limited use. Optimizing that code path based on the benchmark would likely have negative performance implications for the common case.

Created attachment 141771 [details] Patch. Minor ChangeLog cleanup. Minor ChangeLog cleanup.