Bug 60318 - [meta] Switch away from integers representing pixels for layout/event handling/rendering
Summary: [meta] Switch away from integers representing pixels for layout/event handlin...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Levi Weintraub
URL:
Keywords:
Depends on: 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
Blocks: 85303 85532 22759 23170 28804 39884 39924 54018 82529 84616 85534 85651 88259
  Show dependency treegraph
 
Reported: 2011-05-05 17:08 PDT by Levi Weintraub
Modified: 2012-06-28 10:35 PDT (History)
25 users (show)

See Also:


Attachments
zooming test case (1.62 KB, text/html)
2011-06-21 11:31 PDT, Emil A Eklund
no flags Details
perf test case (8.18 KB, text/html)
2011-06-21 11:32 PDT, Emil A Eklund
no flags Details
zoom perf test case (1.95 KB, text/html)
2011-06-21 11:32 PDT, Emil A Eklund
no flags Details
Proof of concept (665.78 KB, patch)
2011-06-21 11:36 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch switching LayoutUnit int -> float. Do not review. (1.15 KB, patch)
2011-07-11 15:17 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (63.05 KB, patch)
2012-04-27 13:47 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (64.03 KB, patch)
2012-04-27 14:29 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (66.07 KB, patch)
2012-04-27 15:54 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (52.81 KB, patch)
2012-05-02 15:49 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch for landing (41.31 KB, patch)
2012-05-03 11:15 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch for landing (42.08 KB, patch)
2012-05-03 12:09 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch. Minor ChangeLog cleanup. (3.08 KB, patch)
2012-05-14 12:45 PDT, Michael Brüning
rniwa: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Levi Weintraub 2011-05-05 17:08:15 PDT
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 Emil A Eklund 2011-06-21 11:31:40 PDT
Created attachment 98021 [details]
zooming test case
Comment 2 Emil A Eklund 2011-06-21 11:32:07 PDT
Created attachment 98022 [details]
perf test case
Comment 3 Emil A Eklund 2011-06-21 11:32:21 PDT
Created attachment 98023 [details]
zoom perf test case
Comment 4 Levi Weintraub 2011-06-21 11:36:23 PDT
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.
Comment 5 Levi Weintraub 2011-06-23 10:34:35 PDT
(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.
Comment 6 Emil A Eklund 2011-07-11 15:17:43 PDT
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.
Comment 7 Levi Weintraub 2012-04-27 13:47:01 PDT
Created attachment 139264 [details]
Patch
Comment 8 Philippe Normand 2012-04-27 14:05:52 PDT
Comment on attachment 139264 [details]
Patch

Attachment 139264 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12553343
Comment 9 Early Warning System Bot 2012-04-27 14:11:32 PDT
Comment on attachment 139264 [details]
Patch

Attachment 139264 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12567166
Comment 10 Early Warning System Bot 2012-04-27 14:13:06 PDT
Comment on attachment 139264 [details]
Patch

Attachment 139264 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12551311
Comment 11 Build Bot 2012-04-27 14:18:52 PDT
Comment on attachment 139264 [details]
Patch

Attachment 139264 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12567170
Comment 12 Levi Weintraub 2012-04-27 14:29:42 PDT
Created attachment 139280 [details]
Patch
Comment 13 Early Warning System Bot 2012-04-27 15:34:40 PDT
Comment on attachment 139280 [details]
Patch

Attachment 139280 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12548366
Comment 14 Levi Weintraub 2012-04-27 15:54:13 PDT
Created attachment 139296 [details]
Patch
Comment 15 Eric Seidel (no email) 2012-04-27 16:08:35 PDT
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 16 Early Warning System Bot 2012-04-27 17:28:50 PDT
Comment on attachment 139296 [details]
Patch

Attachment 139296 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12550411
Comment 17 Darin Adler 2012-04-27 18:19:59 PDT
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 18 WebKit Review Bot 2012-04-28 02:52:56 PDT
Comment on attachment 139296 [details]
Patch

Attachment 139296 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12550528
Comment 19 Levi Weintraub 2012-05-02 15:49:42 PDT
Created attachment 139897 [details]
Patch
Comment 20 Eric Seidel (no email) 2012-05-02 16:50:14 PDT
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. :)
Comment 21 Levi Weintraub 2012-05-03 11:15:37 PDT
Created attachment 140054 [details]
Patch for landing
Comment 22 Levi Weintraub 2012-05-03 12:09:18 PDT
Created attachment 140063 [details]
Patch for landing
Comment 23 Levi Weintraub 2012-05-03 12:10:14 PDT
(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)
Comment 24 Levi Weintraub 2012-05-03 14:06:44 PDT
Committed r116009: <http://trac.webkit.org/changeset/116009>
Comment 25 Allan Sandfeld Jensen 2012-05-04 06:36:54 PDT
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.
Comment 26 Allan Sandfeld Jensen 2012-05-04 06:40:21 PDT
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 Emil A Eklund 2012-05-04 09:20:47 PDT
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.
Comment 29 Darin Adler 2012-05-06 16:49:18 PDT
(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 Ryosuke Niwa 2012-05-06 17:02:42 PDT
(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 Emil A Eklund 2012-05-07 15:33:38 PDT
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 Emil A Eklund 2012-05-10 14:40:50 PDT
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 Emil A Eklund 2012-05-11 11:01:45 PDT
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 Michael Brüning 2012-05-14 12:45:44 PDT
Created attachment 141771 [details]
Patch. Minor ChangeLog cleanup.

Minor ChangeLog cleanup.