Summary: | Fix performance regression for floats caused by LayoutUnit change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Emil A Eklund <eae> | ||||||||||||
Component: | Layout and Rendering | Assignee: | 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
Emil A Eklund
2012-05-07 15:22:13 PDT
Created attachment 140605 [details]
Patch
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 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 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 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. 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%. 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. Created attachment 140613 [details]
Patch for landing
Comment on attachment 140613 [details] Patch for landing Clearing flags on attachment: 140613 Committed r116392: <http://trac.webkit.org/changeset/116392> All reviewed patches have been landed. Closing bug. Unfortunately, this doesn't seem to have worked. :( 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! Created attachment 140820 [details]
Patch
Another try at addressing the performance regressions after 116009.
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.
(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 on attachment 140820 [details] Patch Attachment 140820 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12652527 Comment on attachment 140820 [details] Patch Attachment 140820 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12651479 Comment on attachment 140820 [details] Patch Attachment 140820 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12647495 Comment on attachment 140820 [details] Patch Attachment 140820 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12656470 Created attachment 140972 [details]
Patch for landing
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.
Created attachment 140985 [details]
Patch for landing
Comment on attachment 140985 [details] Patch for landing Clearing flags on attachment: 140985 Committed r116549: <http://trac.webkit.org/changeset/116549> All reviewed patches have been landed. Closing bug. (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. |