Bug 85834

Summary: Fix performance regression for floats caused by LayoutUnit change
Product: WebKit Reporter: Emil A Eklund <eae>
Component: Layout and RenderingAssignee: Levi Weintraub <leviw>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, leviw, ojan, rakuco, rniwa, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing
none
Patch
none
Patch for landing
none
Patch for landing none

Comment 1 Emil A Eklund 2012-05-07 15:28:02 PDT
Created attachment 140605 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-05-07 15:33:10 PDT
Comment on attachment 140605 [details]
Patch

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

OK.  So does this get us back the whole speed loss?

> Source/WebCore/platform/FractionalLayoutUnit.h:324
> +#if ENABLE(SUBPIXEL_LAYOUT)

Presumably there is still a regression for ports with this enabled?
Comment 3 Eric Seidel (no email) 2012-05-07 15:33:32 PDT
Comment on attachment 140605 [details]
Patch

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

>> Source/WebCore/platform/FractionalLayoutUnit.h:324
>> +#if ENABLE(SUBPIXEL_LAYOUT)
> 
> Presumably there is still a regression for ports with this enabled?

Also, this multiply is not very bounded anymore... :)
Comment 4 Emil A Eklund 2012-05-07 15:38:42 PDT
Comment on attachment 140605 [details]
Patch

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

It should get us back most of the lost performance. I'll post numbers before committing.

>>> Source/WebCore/platform/FractionalLayoutUnit.h:324
>>> +#if ENABLE(SUBPIXEL_LAYOUT)
>> 
>> Presumably there is still a regression for ports with this enabled?
> 
> Also, this multiply is not very bounded anymore... :)

Yes, we plan to address that separately before turning on the flag. We have a couple of ideas but need some more time validating them.
Comment 5 Ryosuke Niwa 2012-05-07 15:40:34 PDT
Comment on attachment 140605 [details]
Patch

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

> Source/WebCore/ChangeLog:19
> +        Disable the use of 64bit (long long) math ni the case where the fraction

Nit: ni.
Comment 6 Emil A Eklund 2012-05-07 16:02:05 PDT
For floats_50_100_nested and floats_20_100_nested this patch seems to get back the entire speed loss. For floats_2_100_nested on the other hand we get back about 60%.
Comment 7 Emil A Eklund 2012-05-07 16:05:56 PDT
Unless anyone objects I plan to commit this and see what our perf bots thinks. If it fails to adequately address the regression I'll roll out the LayoutUnit change.
Comment 8 Emil A Eklund 2012-05-07 16:10:03 PDT
Created attachment 140613 [details]
Patch for landing
Comment 9 WebKit Review Bot 2012-05-07 21:11:31 PDT
Comment on attachment 140613 [details]
Patch for landing

Clearing flags on attachment: 140613

Committed r116392: <http://trac.webkit.org/changeset/116392>
Comment 10 WebKit Review Bot 2012-05-07 21:11:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Ojan Vafai 2012-05-08 11:11:54 PDT
Unfortunately, this doesn't seem to have worked. :(
Comment 12 Darin Adler 2012-05-08 12:46:15 PDT
Comment on attachment 140613 [details]
Patch for landing

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

> Source/WebCore/ChangeLog:11
> +        Fix performance regression caused by r116009 by disabling the use of
> +        64bit math in FractionalLayoutUnit, simplifying the pixelSnappedMaxX/Y
> +        math, inlining a couple of methods and replacing the literal 0 (zero)
> +        with ZERO_LAYOUT_UNIT.

Did you verify that the 0 -> ZERO_LAYOUT_UNIT change actually affected performance? I find that highly dubious, so I want to be sure it actually happened!
Comment 13 Levi Weintraub 2012-05-08 16:59:59 PDT
Created attachment 140820 [details]
Patch

Another try at addressing the performance regressions after 116009.
Comment 14 WebKit Review Bot 2012-05-08 17:02:10 PDT
Attachment 140820 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/FractionalLayoutRect.h:187:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Levi Weintraub 2012-05-08 17:15:31 PDT
(In reply to comment #14)
> Attachment 140820 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> Source/WebCore/platform/graphics/FractionalLayoutRect.h:187:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
> Total errors found: 1 in 4 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Thanks for the review, Ojan. I'll fix the style error and am waiting for the EWS bots to validate since I mucked with build files.
Comment 16 Build Bot 2012-05-08 17:31:19 PDT
Comment on attachment 140820 [details]
Patch

Attachment 140820 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12652527
Comment 17 Adele Peterson 2012-05-08 17:47:05 PDT
<rdar://problem/11411562>
Comment 18 Build Bot 2012-05-08 18:30:39 PDT
Comment on attachment 140820 [details]
Patch

Attachment 140820 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12651479
Comment 19 Build Bot 2012-05-08 18:40:45 PDT
Comment on attachment 140820 [details]
Patch

Attachment 140820 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12647495
Comment 20 Gyuyoung Kim 2012-05-08 18:58:53 PDT
Comment on attachment 140820 [details]
Patch

Attachment 140820 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12656470
Comment 21 Levi Weintraub 2012-05-09 10:38:08 PDT
Created attachment 140972 [details]
Patch for landing
Comment 22 WebKit Review Bot 2012-05-09 11:07:19 PDT
Attachment 140972 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebCore/ChangeLog:18:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 Levi Weintraub 2012-05-09 11:29:25 PDT
Created attachment 140985 [details]
Patch for landing
Comment 24 WebKit Review Bot 2012-05-09 12:35:23 PDT
Comment on attachment 140985 [details]
Patch for landing

Clearing flags on attachment: 140985

Committed r116549: <http://trac.webkit.org/changeset/116549>
Comment 25 WebKit Review Bot 2012-05-09 12:35:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Levi Weintraub 2012-05-09 12:37:46 PDT
(In reply to comment #25)
> All reviewed patches have been landed.  Closing bug.

I'll be monitoring the perf bots and if we don't see improvement, I'll roll back 116009 pending further investigation.