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
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
<rdar://problem/11411562>
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>
(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.