Bug 60318 – [meta] Switch away from integers representing pixels for layout/event handling/rendering

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Levi Weintraub]

Using integers that represent pixels for layout leads to rounding errors when zooming or transforms are applied. This bug will track our progress towards replacing this with something that solves these problems. What this something is hasn't been determined.

[Emil A Eklund]

Created attachment 98021 [details]
zooming test case

[Emil A Eklund]

Created attachment 98022 [details]
perf test case

[Emil A Eklund]

Created attachment 98023 [details]
zoom perf test case

[Levi Weintraub]

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.

[Levi Weintraub]

(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.

[Emil A Eklund]

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.

[Levi Weintraub]

Created attachment 139264 [details]
Patch

[Philippe Normand]

Comment on attachment 139264 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 139264 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 139264 [details]
Patch

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

[Build Bot]

Comment on attachment 139264 [details]
Patch

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

[Levi Weintraub]

Created attachment 139280 [details]
Patch

[Early Warning System Bot]

Comment on attachment 139280 [details]
Patch

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

[Levi Weintraub]

Created attachment 139296 [details]
Patch

[Eric Seidel (no email)]

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);

?

[Early Warning System Bot]

Comment on attachment 139296 [details]
Patch

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

[Darin Adler]

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.

[WebKit Review Bot]

Comment on attachment 139296 [details]
Patch

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

[Levi Weintraub]

Created attachment 139897 [details]
Patch

[Eric Seidel (no email)]

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. :)

[Levi Weintraub]

Created attachment 140054 [details]
Patch for landing

[Levi Weintraub]

Created attachment 140063 [details]
Patch for landing

[Levi Weintraub]

(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)

[Levi Weintraub]

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

[Allan Sandfeld Jensen]

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.

[Allan Sandfeld Jensen]

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.

[Emil A Eklund]

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.

[Ryosuke Niwa]

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.

[Darin Adler]

(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.

[Ryosuke Niwa]

(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).

[Emil A Eklund]

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.

[Emil A Eklund]

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

[Emil A Eklund]

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.

[Michael Brüning]

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

Minor ChangeLog cleanup.