Bug 85834 – Fix performance regression for floats caused by LayoutUnit change

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Emil A Eklund]

The LayoutUnit (r116009) caused the nested float tests and the chromium http-bloat tests to regress.

http://webkit-perf.appspot.com/graph.html#tests=[[472030,2001,3001],[393045,2001,3001],[393045,2001,173262],[363057,2001,32196],[363057,2001,3001],[472030,2001,173262],[363057,2001,173262],[472030,2001,32196],[393045,2001,32196]]&sel=1335802788243,1336407588243&displayrange=7&datatype=running

http://build.chromium.org/f/chromium/perf/linux-release/bloat-http/report.html?history=150&rev=-1

[Emil A Eklund]

Created attachment 140605 [details]
Patch

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

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... :)

[Emil A Eklund]

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.

[Ryosuke Niwa]

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.

[Emil A Eklund]

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%.

[Emil A Eklund]

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.

[Emil A Eklund]

Created attachment 140613 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 140613 [details]
Patch for landing

Clearing flags on attachment: 140613

Committed r116392: <http://trac.webkit.org/changeset/116392>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ojan Vafai]

Unfortunately, this doesn't seem to have worked. :(

[Darin Adler]

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!

[Levi Weintraub]

Created attachment 140820 [details]
Patch

Another try at addressing the performance regressions after 116009.

[WebKit Review Bot]

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.

[Levi Weintraub]

(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.

[Build Bot]

Comment on attachment 140820 [details]
Patch

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

[Adele Peterson]

<rdar://problem/11411562>

[Build Bot]

Comment on attachment 140820 [details]
Patch

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

[Build Bot]

Comment on attachment 140820 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 140820 [details]
Patch

Attachment 140820 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12656470

[Levi Weintraub]

Created attachment 140972 [details]
Patch for landing

[WebKit Review Bot]

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.

[Levi Weintraub]

Created attachment 140985 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 140985 [details]
Patch for landing

Clearing flags on attachment: 140985

Committed r116549: <http://trac.webkit.org/changeset/116549>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Levi Weintraub]

(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.