Bug 93513 – Accumulating LayoutUnits with floats for determining block preferred width can lead to wrapping

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

[Levi Weintraub]

When computing inline preferred logical widths, we accumulate our children's widths using floating point numbers, but when adding the width of floating elements, we can truncate up to 1/60th of a pixel. We should be using the nearest larger float instead of the nearest smaller one. You can see the result of this wrapping in the top-nav bar on developer.apple.com.

[Levi Weintraub]

Created attachment 157338 [details]
Patch

[Levi Weintraub]

Ping.

[Allan Sandfeld Jensen]

Interesting problem, but I think we might want to consider if we want to solve this instead by changing the fixed-point divisor to 64. By using a power of 2, it is not only much faster, but it means we will never loose precision when converting to float. 

From a API point of view it is just much less confusing if we can safely assume that float will be more precise than fixed points (for most of the used range anyway).

[Levi Weintraub]

(In reply to comment #3)
> Interesting problem, but I think we might want to consider if we want to solve this instead by changing the fixed-point divisor to 64. By using a power of 2, it is not only much faster, but it means we will never loose precision when converting to float. 
> 
> From a API point of view it is just much less confusing if we can safely assume that float will be more precise than fixed points (for most of the used range anyway).

60 was used because it's easily dived by 2, 3, 4, 5, 6, and 10. I'm not opposed to changing it, but we shouldn't write code that's dependent upon the divisor (it could be changed later), and I believe we'd want to do some testing with 64 before settling on it.

[Allan Sandfeld Jensen]

(In reply to comment #4)
> 60 was used because it's easily dived by 2, 3, 4, 5, 6, and 10. I'm not opposed to changing it, but we shouldn't write code that's dependent upon the divisor (it could be changed later), and I believe we'd want to do some testing with 64 before settling on it.

I only brought it up because this bug highlighted a consequence I had never considered before of using divisors that are not powers of 2, and I just don't like the fact that we might lose precision in places we didn't consider before.

If we want to keep the divisors, but want to ensure we never lose precision like that, another alternative could be get rid of float, but would either require fully enabling subpixel layout, or make fixed point available when explicitly used even without subpixel layout enabled.

Btw, it is not that I think there is anything wrong with your solution, this line of thinking it mostly to think of ways to avoid similar problems elsewhere.

[Levi Weintraub]

(In reply to comment #5)
> (In reply to comment #4)
> > 60 was used because it's easily dived by 2, 3, 4, 5, 6, and 10. I'm not opposed to changing it, but we shouldn't write code that's dependent upon the divisor (it could be changed later), and I believe we'd want to do some testing with 64 before settling on it.
> 
> I only brought it up because this bug highlighted a consequence I had never considered before of using divisors that are not powers of 2, and I just don't like the fact that we might lose precision in places we didn't consider before.
> 
> If we want to keep the divisors, but want to ensure we never lose precision like that, another alternative could be get rid of float, but would either require fully enabling subpixel layout, or make fixed point available when explicitly used even without subpixel layout enabled.
> 
> Btw, it is not that I think there is anything wrong with your solution, this line of thinking it mostly to think of ways to avoid similar problems elsewhere.

Your thoughts are always appreciated, don't worry :) I just wanted to give background on the current value. We'd considered other values, of course, and had particularly figured that 64 could be used for Mobile, where the performance is more critical.

Perhaps I'm ignorant, but are you implying that we won't be subject to lost precision if we went with a power of two? It seems to me that when we're accumulating large numbers and switching between fixed and floating point we'll always be subject to lost precision.

[Allan Sandfeld Jensen]

(In reply to comment #6)
> Perhaps I'm ignorant, but are you implying that we won't be subject to lost precision if we went with a power of two? It seems to me that when we're accumulating large numbers and switching between fixed and floating point we'll always be subject to lost precision.

If we use a power of two divisor most numbers that can be represented by our layout units can also be represented accurately by floating point, because floating point always uses divisors of two. Floating point can on the other hand not represent division by 10 accurately, so our layout unit can currently represent 0.1 accurately, which is something even a double can not. Which is why we end up losing accuracy when converting from fixed point to floating point, even for ordinary size numbers.

Of course even if we switch to 64 instead of 60, there will be some numbers where fixed point and floating point doesn't match. Fixed point will have better accuracy at higher numbers right up till they overflows, while floating point will have increasingly worse accuracy at higher numbers at the benefit of not overflowing. So we could end up with similar problems like this one for very large numbers, but only in constructed laboratory cases I think.

[Eric Seidel (no email)]

Comment on attachment 157338 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157338&action=review

Looks fine.

> Source/WebCore/rendering/RenderBlock.cpp:3750
> +        // FIXME: LayoutUnit::epsilon is probably only necessary here due to lost precision elsewhere
> +        while (logicalRightOffsetForLine(logicalTopOffset, logicalRightOffset, false, &heightRemainingRight) - floatLogicalLeft + LayoutUnit::epsilon() < floatLogicalWidth) {

How do we fix/track down this fixme?

> Source/WebCore/rendering/RenderBlock.cpp:3765
> +        // FIXME: LayoutUnit::epsilon is probably only necessary here due to lost precision elsewhere

Same question.  What's the plan for fixing this?

[Levi Weintraub]

Created attachment 158368 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 158368 [details]
Patch for landing

Clearing flags on attachment: 158368

Committed r125591: <http://trac.webkit.org/changeset/125591>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.