Bug 12866 – REGRESSION: BenchJS test 6 is 12% slower in TOT than Safari 2.0.4

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Maciej Stachowiak]

The 24Fun benchmark's flying letters test (test 6) is 6% slower in current WebKit than Safari 2.0.4.

[Maciej Stachowiak]

<rdar://problem/5018673>

[Maciej Stachowiak]

I think this is actually about 12% slower, not 6%.

[Maciej Stachowiak]

Created attachment 13347 [details]
partial fix - 2% speedup on test 6

Comment on attachment 13347 [details]
partial fix - 2% speedup on test 6

r=me

[Maciej Stachowiak]

Created attachment 13349 [details]
partial fix 2 - 12.5% speedup

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

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

[Dave Hyatt]

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.

[Maciej Stachowiak]

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.

Opps, looks like attachment 13349 [details] has caused a regression <http://build.webkit.org/results/post-commit-pixel-powerpc-mac-os-x/3698/fast/overflow/scrollRevealButton-diffs.html>.

[Dave Hyatt]

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.

[Dave Hyatt]

Created attachment 13352 [details]
Minor tweak.  I think printing probably should setMinMaxKnown(false)

[Dave Hyatt]

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.

[Darin Adler]

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.

[Maciej Stachowiak]

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

[Maciej Stachowiak]

I did the performance testing and it is a 16% speedup on Test 6 of the BenchJS benchmark.

[Maciej Stachowiak]

Comment on attachment 13353 [details]
Fix adjustViewSize to not be slow.

r=me

[Maciej Stachowiak]

Hyatt landed his latest patch.

[David Kilzer (:ddkilzer)]

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