Bug 96139 - Change FractionalLayoutUnit denominator to 64 to reduce precision loss when converting to floating point
Summary: Change FractionalLayoutUnit denominator to 64 to reduce precision loss when c...
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: Emil A Eklund
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-07 13:39 PDT by Emil A Eklund
Modified: 2012-09-26 10:33 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1023.39 KB, patch)
2012-09-07 14:53 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (1023.38 KB, patch)
2012-09-10 10:50 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (1.00 MB, patch)
2012-09-25 14:59 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emil A Eklund 2012-09-07 13:39:59 PDT
We currently use a denominator of 60 in FractionalLayoutUnit, this causes a loss of precision when converting to floating point.
By changing the denominator to 64 the values can better be represented as floating point (without loosing any precision for many values), this in turn allows us to remove the tolerance hack in the line break logic and avoids problems caused by this precision for web sites that do their own layout based on element measurements.
Comment 1 Emil A Eklund 2012-09-07 14:53:10 PDT
Created attachment 162877 [details]
Patch
Comment 2 WebKit Review Bot 2012-09-08 02:26:09 PDT
Comment on attachment 162877 [details]
Patch

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

New failing tests:
fast/block/float/overhanging-tall-block.html
fast/sub-pixel/large-sizes.html
Comment 3 Allan Sandfeld Jensen 2012-09-10 05:20:52 PDT
Good stuff. In addition it should also make it faster to do conversions from FractionalLayoutUnit to integer.

The two tests failing seems to be the ones that overflow the integers, so that was to be expected.
Comment 4 Emil A Eklund 2012-09-10 08:31:37 PDT
(In reply to comment #3)
> Good stuff. In addition it should also make it faster to do conversions from FractionalLayoutUnit to integer.

Yeah, you have some nice permanence benefits as well. Bit shifting all the way.


> The two tests failing seems to be the ones that overflow the integers, so that was to be expected.

Right, I'll upload a new patch with fixes for those once we decide whether this is a good idea or not.

Thanks.
Comment 5 Emil A Eklund 2012-09-10 10:50:03 PDT
Created attachment 163165 [details]
Patch
Comment 6 Eric Seidel (no email) 2012-09-10 11:17:07 PDT
Comment on attachment 163165 [details]
Patch

What was the original logic behind 60?
Comment 7 Emil A Eklund 2012-09-10 11:19:10 PDT
(In reply to comment #6)
> (From update of attachment 163165 [details])
> What was the original logic behind 60?

Evenly divisible by 2, 3, 4, 5, 6 and 10.

Given the default zoom levels this doesn't actually matter as much as we thought it would.
Comment 8 WebKit Review Bot 2012-09-10 19:31:39 PDT
Comment on attachment 163165 [details]
Patch

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

New failing tests:
fast/block/float/overhanging-tall-block.html
fast/sub-pixel/large-sizes.html
Comment 9 Emil A Eklund 2012-09-25 14:59:35 PDT
Created attachment 165681 [details]
Patch
Comment 10 Eric Seidel (no email) 2012-09-25 15:12:09 PDT
Comment on attachment 165681 [details]
Patch

Looks reasonable.
Comment 11 Emil A Eklund 2012-09-25 15:53:18 PDT
Thanks Eric. I'll try to commit this when the tree is quiet to avoid painful merges.
Comment 12 Levi Weintraub 2012-09-25 15:55:42 PDT
Super exciting. This should pretty much eliminate the perf hit sub-pixel causes :D
Comment 13 Build Bot 2012-09-25 18:13:29 PDT
Comment on attachment 165681 [details]
Patch

Attachment 165681 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14027320

New failing tests:
fast/block/float/overhanging-tall-block.html
Comment 14 Allan Sandfeld Jensen 2012-09-26 02:50:15 PDT
(In reply to comment #13)
> (From update of attachment 165681 [details])
> Attachment 165681 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/14027320
> 
> New failing tests:
> fast/block/float/overhanging-tall-block.html

When I had a patch that changed these tests, I just marked them as FAIL, and then later fixed the baseline using the garden-o-matic tool. It seemed easier that way than setting up all the different environments needed yourself.
Comment 15 Emil A Eklund 2012-09-26 09:28:22 PDT
Committed r129656: <http://trac.webkit.org/changeset/129656>