Bug 98913 – Change transformation precision from double to float

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[Gabor Rapcsanyi]

I created this bug to discuss do we really need the double precision here. If not we should change it back to float or make a typedef or something. I would like to do that because there are some functions here which could be optimized with ARM SIMD.

[Simon Fraser (smfr)]

It used to be floats and was converted to doubles. Did you go through the file history?

[Gabor Rapcsanyi]

(In reply to comment #1)
> It used to be floats and was converted to doubles. Did you go through the file history?

There were a lot of file renaming and code moving etc but yes I checked the history of some files, but I still don't see when it changes to double from float.
You mentioned in your mail this bug (https://bugs.webkit.org/show_bug.cgi?id=6868) which I checked as well but when I watched the patch closer I still saw double parameters which were there before it.
So I tracked back the AffineTransform.h to see when it started using double but I've just found this:
http://trac.webkit.org/browser/trunk/WebCore/kwq/KWQWMatrix.h?annotate=blame&rev=9585
Which works with doubles.

I also checked the history of TransformationMatrix.h and I found this:
http://trac.webkit.org/browser/trunk/WebCore/rendering/style/RenderStyle.h?annotate=blame&rev=34535
It contains the MatrixTransformOperation class which is still using double parameters.

So I'm a little bit confused when we started using doubles for the transformations and why. But to make it clear I would like to change all doubles in Source/WebCore/platform/graphics/transforms to float or maybe it would be better to a typedef.

[Brent Fulgham]

One thing we should be careful of is propagation of rounding errors.  For WebKit's purposes, it may not matter as the various floating point operations eventually convert into whole pixel values, but I have seen cases in scientific computing where the vagaries of floating point/double conversion lead to difficult to debug problems.

If we aren't doing much math where floating point values are being raised to large powers, or trigonometric functions results are being chained together there may be little downside.

I guess the layout tests will show whether this change has any correctness implications.

[Simon Fraser (smfr)]

(In reply to comment #3)
> One thing we should be careful of is propagation of rounding errors.  For WebKit's purposes, it may not matter as the various floating point operations eventually convert into whole pixel values, but I have seen cases in scientific computing where the vagaries of floating point/double conversion lead to difficult to debug problems.
> 
> If we aren't doing much math where floating point values are being raised to large powers, or trigonometric functions results are being chained together there may be little downside.

There are certainly cases where rounding errors would be compounded; we map points and rects through accumulated transforms for hit testing etc, and it's not hard to create content (like almost edge-on layers) where rounding matters.

[Gabor Rapcsanyi]

Created attachment 168180 [details]
Draft

I made a draft patch to see what needs to be changed with the new precision and I tried it on Qt port.
Surprisingly there are just 47 fails mostly SVG tests. And these fails are because of -0.0 and 0.0 difference or just one or two pixel differences.

[WebKit Review Bot]

Attachment 168180 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/wtf/Platform.h', u'Source/WebCo..." exit_code: 1
Source/WebCore/platform/graphics/transforms/ScaleTransformOperation.cpp:36:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/transforms/ScaleTransformOperation.cpp:37:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/transforms/AffineTransform.h:64:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/transforms/AffineTransform.h:64:  The parameter name "c" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/transforms/AffineTransform.h:64:  The parameter name "e" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/transforms/AffineTransform.h:64:  The parameter name "f" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/transforms/AffineTransform.h:70:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/transforms/AffineTransform.h:70:  The parameter name "c" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/transforms/AffineTransform.h:70:  The parameter name "e" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/transforms/AffineTransform.h:70:  The parameter name "f" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:76:  SMALL_NUMBER is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:266:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:82:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:83:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:84:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:103:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:104:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:105:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:253:  The parameter name "p" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 19 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 168180 [details]
Draft

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

[Build Bot]

Comment on attachment 168180 [details]
Draft

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

[Peter Beverloo (cr-android ews)]

Comment on attachment 168180 [details]
Draft

Attachment 168180 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14264040

[Build Bot]

Comment on attachment 168180 [details]
Draft

Attachment 168180 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14256619

[Simon Hausmann]

I strongly suggest against a typedef.

We've made this mistake in Qt, we introduced a "qreal" type that was typedef'ed to double on "desktop" platforms and "float" on "embedded"/"mobile" platforms (ARM based mostly). I'm saying it was a mistake because the introduced inconsistency brought in many issues.

We've had a thread on the Qt development mailing list a while ago about trying to get rid of the typedef. The thread starts here:

http://lists.qt-project.org/pipermail/development/2012-February/002015.html

[Gabor Rapcsanyi]

Created attachment 168725 [details]
transform.html

Performance test for transformations.

[Gabor Rapcsanyi]

(In reply to comment #11)
> I strongly suggest against a typedef.
> 
> We've made this mistake in Qt, we introduced a "qreal" type that was typedef'ed to double on "desktop" platforms and "float" on "embedded"/"mobile" platforms (ARM based mostly). I'm saying it was a mistake because the introduced inconsistency brought in many issues.
> 
> We've had a thread on the Qt development mailing list a while ago about trying to get rid of the typedef. The thread starts here:
> 
> http://lists.qt-project.org/pipermail/development/2012-February/002015.html

Ok, I've read this thread and now I see 2 solutions, (a) is to make template classes as somebody suggested on the Qt mailing list and (b) is that change everything to floats in transformations.

I think the first thing what we should discuss is where we really need the double precision. So if there is a part of WebKit which really needs this extra precision then we shouldn't chose (b).

The other thing is that your system APIs (Darin said OS X as well) using doubles so the conversions may take the time save of floats.
Memory usage is another important thing and I would like to do some measures to see the benefit of floats.

I uploaded a test (transform.html) which I wrote to see the performance gain of floats. It's using css transformations on small rectangles. The bottleneck was of course the matrix multiplication in transformations, but unfortunately I didn't see any performance differences between the doubles or floats. 

I want to do some memory measurements as well. I hope I would see more differences there.

[Simon Fraser (smfr)]

(In reply to comment #12)
> Created an attachment (id=168725) [details]
> transform.html
> 
> Performance test for transformations.

Profiling this shows that it spends 12% of its time in cssText(), and about 1.5% in TransformationMatrix::multiply(), on desktop at least. So multiply() is far from being the bottleneck. dtoa() takes more time.

[Gabor Rapcsanyi]

(In reply to comment #14)
> (In reply to comment #12)
> > Created an attachment (id=168725) [details] [details]
> > transform.html
> > 
> > Performance test for transformations.
> 
> Profiling this shows that it spends 12% of its time in cssText(), and about 1.5% in TransformationMatrix::multiply(), on desktop at least. So multiply() is far from being the bottleneck. dtoa() takes more time.

Well I did some more investigation in this area.
I measured the speed and memory gain with this transform.html on a Pandaboard.

As I wrote above there was no big speed differences between the float and double types even on this specific test. I changed the matrix multiplication to NEON code to see the improvement but it wasn't too much.

Speed test:
Double: ~6.03 fps
Float:  ~6.32 fps
NEON:   ~6.6 fps

I also did some memory measurements on this test with Smaps.

libWebCore memory usage:
Float: 14 540 kB
Double:14 580 kB
Difference: 40 kB save

Heap usage:
Float: 59 352 kB
Double:59 792 kB
Difference: 440 kb save

Sum memory usage:
Float: 95 864 kB
Double:96 368 kB
Difference: 504 kb save

As we can see the memory difference is not too much.

Summarize all of these results I think changing to float the doubles is not worth. There weren't too much performance gain even with the neonized matrix multiplication on a specific test. It would only worth because of the memory savings but it's not too much either.

So I'll close this bug with wontfix if there are no objections.

[Benjamin Poulain]

> So I'll close this bug with wontfix if there are no objections.

Sounds good to me.

Something you may wanna do is check if the compiler uses VFP vector and mixed-mode operations. They take the same time per operations but at least it takes fewer opcodes.

[Gabor Rapcsanyi]

(In reply to comment #16)
> > So I'll close this bug with wontfix if there are no objections.
> 
> Sounds good to me.
> 
> Something you may wanna do is check if the compiler uses VFP vector and mixed-mode operations. They take the same time per operations but at least it takes fewer opcodes.

I tried it with VFP and it was 6.38 fps so a little bit faster.
Now I'm closing the bug.