Bug 12866 - REGRESSION: BenchJS test 6 is 12% slower in TOT than Safari 2.0.4
Summary: REGRESSION: BenchJS test 6 is 12% slower in TOT than Safari 2.0.4
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Maciej Stachowiak
URL: http://www.24fun.com/downloadcenter/b...
Keywords: InRadar, NeedsReduction, Regression
Depends on:
Blocks:
 
Reported: 2007-02-23 00:39 PST by Maciej Stachowiak
Modified: 2008-02-28 16:05 PST (History)
2 users (show)

See Also:


Attachments
partial fix - 2% speedup on test 6 (3.42 KB, patch)
2007-02-23 07:31 PST, Maciej Stachowiak
mitz: review+
Details | Formatted Diff | Diff
partial fix 2 - 12.5% speedup (4.10 KB, patch)
2007-02-23 07:54 PST, Maciej Stachowiak
mitz: review+
Details | Formatted Diff | Diff
Patch to rewrite RenderView::layout() (5.96 KB, patch)
2007-02-23 14:35 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff
Minor tweak. I think printing probably should setMinMaxKnown(false) (6.02 KB, patch)
2007-02-23 14:43 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff
Fix adjustViewSize to not be slow. (6.40 KB, patch)
2007-02-23 14:53 PST, Dave Hyatt
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Maciej Stachowiak 2007-02-23 00:39:18 PST
The 24Fun benchmark's flying letters test (test 6) is 6% slower in current WebKit than Safari 2.0.4.
Comment 1 Maciej Stachowiak 2007-02-23 00:44:38 PST
<rdar://problem/5018673>
Comment 2 Maciej Stachowiak 2007-02-23 07:30:17 PST
I think this is actually about 12% slower, not 6%.
Comment 3 Maciej Stachowiak 2007-02-23 07:31:15 PST
Created attachment 13347 [details]
partial fix - 2% speedup on test 6
Comment 4 mitz 2007-02-23 07:34:04 PST
Comment on attachment 13347 [details]
partial fix - 2% speedup on test 6

r=me
Comment 5 Maciej Stachowiak 2007-02-23 07:54:51 PST
Created attachment 13349 [details]
partial fix 2 - 12.5% speedup
Comment 6 mitz 2007-02-23 08:27:32 PST
Comment on attachment 13349 [details]
partial fix 2 - 12.5% speedup

Maciej agreed to reinstate the null checking of m_frameView. Assuming that and a chnage log, r=me
Comment 7 mitz 2007-02-23 10:33:46 PST
In r19761 (that is, without the latest patches), I get a speedup of roughly 25% if I tweak the test so that the layers will have an absolutely positioned div as their parent instead of the body. This demonstrates how bad it is that RenderView::layout() unconditionally lays out all of its positioned objects. In the BenchJS test, this causes each tiny block to relayout, and as a side effect also to invalidate its repaint rect twice (once as a result of updating the inline layout and once as a result of the layer moving).
Comment 8 Dave Hyatt 2007-02-23 13:23:27 PST
I assume this problem was made worse by my fix to make the containing block for positioned elements (with no positioned ancestor) becoming the RenderView rather than the root element.

Comment 9 Maciej Stachowiak 2007-02-23 13:35:54 PST
I think we should fix the unconditional layout problem Mitz mentioned as the regression it causes clearly shows on the profile, even though test 6 is now faster than baseline.
Comment 11 Dave Hyatt 2007-02-23 14:35:05 PST
Created attachment 13351 [details]
Patch to rewrite RenderView::layout()

This will need a lot of testing.  It reworks RenderView::layout as follows:

(1) Avoid always laying out the root (it only does so now if the visible view size changes).
(2) Never setMinMaxKnown to false ever.  Let the normal mechanism for that always be used.
(3) Stop the relayout of positioned objects.  With calcWidth() and calcHeight() already overridden, the initial containing block size was already correct.
(4) Treat the docWidth() and docHeight() as overflow and let the normal layer sizing mechanisms handle sizing the layer.
(5) Removed the flexible box layout hack from RenderView, since nobody is using it any more and it was wrong anyway.
Comment 12 Dave Hyatt 2007-02-23 14:43:57 PST
Created attachment 13352 [details]
Minor tweak.  I think printing probably should setMinMaxKnown(false)
Comment 13 Dave Hyatt 2007-02-23 14:53:56 PST
Created attachment 13353 [details]
Fix adjustViewSize to not be slow.

adjustViewSize was calling into docWidth/Height every time.  This info was cached on the layer (even before this patch), so there's no reason the method can't be fast.
Comment 14 Darin Adler 2007-02-24 16:16:56 PST
Comment on attachment 13353 [details]
Fix adjustViewSize to not be slow.

I reviewed this pretty carefully and it all looks right to me.

I guess the remaining steps are to carefully test to see that this doesn't break anything and to do new performance tests. Not sure who's going to do that.
Comment 15 Maciej Stachowiak 2007-02-24 17:05:51 PST
Comment on attachment 13353 [details]
Fix adjustViewSize to not be slow.

I'm not sure the printing() check is needed, since we'll need layout anyway when printing or coming out of printing. Also, this needs a ChangeLog and should probably be tested thoroughly, including printing. r=me
Comment 16 Maciej Stachowiak 2007-02-24 17:08:07 PST
I did the performance testing and it is a 16% speedup on Test 6 of the BenchJS benchmark.
Comment 17 Maciej Stachowiak 2007-02-24 17:08:21 PST
Comment on attachment 13353 [details]
Fix adjustViewSize to not be slow.

r=me
Comment 18 Maciej Stachowiak 2007-02-25 17:49:53 PST
Hyatt landed his latest patch.
Comment 19 David Kilzer (:ddkilzer) 2008-02-28 16:05:09 PST
Attachment #13347 [details] ("partial fix - 2% speedup on test 6"):
r19827

Attachment #13349 [details] ("partial fix 2 - 12.5% speedup"):
r19828

Attachment #13353 [details] ("Fix adjustViewSize to not be slow."):
r19848
r19849
r19850