Bug 60318 - [meta] Switch away from integers representing pixels for layout/event handling/rendering
: [meta] Switch away from integers representing pixels for layout/event handlin...
Status: RESOLVED FIXED
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 44412 60317 60408 60409 60490 60585 60591 60592 60596 60663 60671 60713 60718 60739 60743 60783 60789 60806 61003 61146 61156 61230 61383 61414 61493 61497 61562 61574 61575 61580 61794 61804 61818 61874 61876 61883 61884 61891 61893 61896 61906 61968 62031 62032 62033 62035 62037 62045 62058 62059 62077 62130 62132 62133 62134 62137 62140 62144 62145 62146 62151 62156 62158 62167 62170 62172 62176 62177 62312 62313 63567 63656 68744 71143 72075 76571 77783 77798 77891 77905 77914 77918 78040 78054 78262 78511 78630 78647 78747 79283 79315 80039 80051 80175 80437 80539 80545 80632 80655 80715 80881 80894 80918 81016 81017 81038 81057 81178 81538 81572 81754 81775 81789 81921 82179 82182 82183 82185 82196 82206 82210 82213 82219 82221 82343 82344 82403 82498 82535 82540 82549 82561 82765 82899 83017 83041 83054 83073 83089 83138 83147 83154 83157 83169 83278 83343 83366 83376 83491 83497 83507 83602 83604 83621 83726 83803 83848 83929 84063 84098 84175 84191 84209 84264 84283 84288 84472 84496 84801 85146 85214 85217 85222 85239 85248 85249 85392 85393 85396 89572 90097 90169 90173
: 22759 23170 28804 39884 39924 54018 82529 84616 85303 85532 85534 85651 88259
  Show dependency treegraph
 
Reported: 2011-05-05 17:08 PST by
Modified: 2012-06-28 10:35 PST (History)


Attachments
zooming test case (1.62 KB, text/html)
2011-06-21 11:31 PST, Emil A Eklund
no flags Details
perf test case (8.18 KB, text/html)
2011-06-21 11:32 PST, Emil A Eklund
no flags Details
zoom perf test case (1.95 KB, text/html)
2011-06-21 11:32 PST, Emil A Eklund
no flags Details
Proof of concept (665.78 KB, patch)
2011-06-21 11:36 PST, Levi Weintraub
no flags Review Patch | Details | Formatted Diff | Diff
Patch switching LayoutUnit int -> float. Do not review. (1.15 KB, patch)
2011-07-11 15:17 PST, Emil A Eklund
no flags Review Patch | Details | Formatted Diff | Diff
Patch (63.05 KB, patch)
2012-04-27 13:47 PST, Levi Weintraub
no flags Review Patch | Details | Formatted Diff | Diff
Patch (64.03 KB, patch)
2012-04-27 14:29 PST, Levi Weintraub
no flags Review Patch | Details | Formatted Diff | Diff
Patch (66.07 KB, patch)
2012-04-27 15:54 PST, Levi Weintraub
no flags Review Patch | Details | Formatted Diff | Diff
Patch (52.81 KB, patch)
2012-05-02 15:49 PST, Levi Weintraub
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (41.31 KB, patch)
2012-05-03 11:15 PST, Levi Weintraub
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (42.08 KB, patch)
2012-05-03 12:09 PST, Levi Weintraub
no flags Review Patch | Details | Formatted Diff | Diff
Patch. Minor ChangeLog cleanup. (3.08 KB, patch)
2012-05-14 12:45 PST, Michael Brüning
rniwa: commit‑queue+
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-05-05 17:08:15 PST
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.
------- Comment #1 From 2011-06-21 11:31:40 PST -------
Created an attachment (id=98021) [details]
zooming test case
------- Comment #2 From 2011-06-21 11:32:07 PST -------
Created an attachment (id=98022) [details]
perf test case
------- Comment #3 From 2011-06-21 11:32:21 PST -------
Created an attachment (id=98023) [details]
zoom perf test case
------- Comment #4 From 2011-06-21 11:36:23 PST -------
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.
------- Comment #5 From 2011-06-23 10:34:35 PST -------
(In reply to comment #4)
> Created an attachment (id=98025) [details] [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.
------- Comment #6 From 2011-07-11 15:17:43 PST -------
Created an attachment (id=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.
------- Comment #7 From 2012-04-27 13:47:01 PST -------
Created an attachment (id=139264) [details]
Patch
------- Comment #8 From 2012-04-27 14:05:52 PST -------
(From update of attachment 139264 [details])
Attachment 139264 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12553343
------- Comment #9 From 2012-04-27 14:11:32 PST -------
(From update of attachment 139264 [details])
Attachment 139264 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12567166
------- Comment #10 From 2012-04-27 14:13:06 PST -------
(From update of attachment 139264 [details])
Attachment 139264 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12551311
------- Comment #11 From 2012-04-27 14:18:52 PST -------
(From update of attachment 139264 [details])
Attachment 139264 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12567170
------- Comment #12 From 2012-04-27 14:29:42 PST -------
Created an attachment (id=139280) [details]
Patch
------- Comment #13 From 2012-04-27 15:34:40 PST -------
(From update of attachment 139280 [details])
Attachment 139280 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12548366
------- Comment #14 From 2012-04-27 15:54:13 PST -------
Created an attachment (id=139296) [details]
Patch
------- Comment #15 From 2012-04-27 16:08:35 PST -------
(From update of attachment 139296 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=139296&action=review

> Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:816
> +                } while (groupRemainingSpace >= 1);

?
------- Comment #16 From 2012-04-27 17:28:50 PST -------
(From update of attachment 139296 [details])
Attachment 139296 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12550411
------- Comment #17 From 2012-04-27 18:19:59 PST -------
(From update of attachment 139296 [details])
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 #18 From 2012-04-28 02:52:56 PST -------
(From update of attachment 139296 [details])
Attachment 139296 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12550528
------- Comment #19 From 2012-05-02 15:49:42 PST -------
Created an attachment (id=139897) [details]
Patch
------- Comment #20 From 2012-05-02 16:50:14 PST -------
(From update of attachment 139897 [details])
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. :)
------- Comment #21 From 2012-05-03 11:15:37 PST -------
Created an attachment (id=140054) [details]
Patch for landing
------- Comment #22 From 2012-05-03 12:09:18 PST -------
Created an attachment (id=140063) [details]
Patch for landing
------- Comment #23 From 2012-05-03 12:10:14 PST -------
(In reply to comment #22)
> Created an attachment (id=140063) [details] [details]
> Patch for landing

(Proactively fixing a build error due to static initializers introduced in r115895)
------- Comment #24 From 2012-05-03 14:06:44 PST -------
Committed r116009: <http://trac.webkit.org/changeset/116009>
------- Comment #25 From 2012-05-04 06:36:54 PST -------
(From update of attachment 140063 [details])
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.
------- Comment #26 From 2012-05-04 06:40:21 PST -------
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 #27 From 2012-05-04 09:20:47 PST -------
(From update of attachment 140063 [details])
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.
------- Comment #29 From 2012-05-06 16:49:18 PST -------
(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.
------- Comment #30 From 2012-05-06 17:02:42 PST -------
(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).
------- Comment #31 From 2012-05-07 15:33:38 PST -------
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.
------- Comment #32 From 2012-05-10 14:40:50 PST -------
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
------- Comment #33 From 2012-05-11 11:01:45 PST -------
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.
------- Comment #34 From 2012-05-14 12:45:44 PST -------
Created an attachment (id=141771) [details]
Patch. Minor ChangeLog cleanup.

Minor ChangeLog cleanup.