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.
It used to be floats and was converted to doubles. Did you go through the file history?
(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.
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.
(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.
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.
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.
Comment on attachment 168180 [details] Draft Attachment 168180 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14258639
Comment on attachment 168180 [details] Draft Attachment 168180 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14258638
Comment on attachment 168180 [details] Draft Attachment 168180 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14264040
Comment on attachment 168180 [details] Draft Attachment 168180 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14256619
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
Created attachment 168725 [details] transform.html Performance test for transformations.
(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.
(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.
(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.
> 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.
(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.