Bug 60318

Summary: [meta] Switch away from integers representing pixels for layout/event handling/rendering
Product: WebKit Reporter: Levi Weintraub <leviw>
Component: Layout and RenderingAssignee: Levi Weintraub <leviw>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, behdad, bugs.webkit.org-19b, darin, ddkilzer, dglazkov, eae, eric, fsamuel, gns, gregsimon, hyatt, isherman, jknotten, leviw, macpherson, menard, pnormand, rakuco, rniwa, scheglov, tonikitoo, webkit.review.bot, xan.lopez, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug 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    
Bug Blocks: 39924, 54018, 85303, 85532, 22759, 23170, 28804, 39884, 82529, 84616, 85534, 85651, 88259    
Attachments:
Description Flags
zooming test case
none
perf test case
none
zoom perf test case
none
Proof of concept
none
Patch switching LayoutUnit int -> float. Do not review.
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch for landing
none
Patch. Minor ChangeLog cleanup. rniwa: commit-queue+

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.