Bug 131801 – [New Multicolumn] Column balancing is much slower than the old multi-column layout on float-multicol.html

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dave Hyatt]

Resize the browser window with fast/multicol/float-multicol.html loaded. It is very very slow. The old multicolumn code is much faster. There is a bit more precision to the balancing with the new code, but I'm not convinced it's worth the performance cost. The latter is probably (just a guess) doing way more relayouts to improve the accuracy.

[Radar WebKit Bug Importer]

<rdar://problem/16656291>

[Morten Stenshorne]

In principle, it shouldn't have to do a lot more relayouts than the number of columns you have (unless you have a bug). I see that your test case has floats, and there's a bug in the new implementation in this regard. I reported it in Blink [1], and I have submitted a patch [2].

[1] https://code.google.com/p/chromium/issues/detail?id=361551
[2] https://codereview.chromium.org/242203003/

[Dave Hyatt]

(In reply to comment #2)
> In principle, it shouldn't have to do a lot more relayouts than the number of columns you have (unless you have a bug). I see that your test case has floats, and there's a bug in the new implementation in this regard. I reported it in Blink [1], and I have submitted a patch [2].
> 
> [1] https://code.google.com/p/chromium/issues/detail?id=361551
> [2] https://codereview.chromium.org/242203003/

I'll look into merging this (see my email to you though that I consider tall-float1.html to be an invalid test case, so I'm not going to merge that). tall-float1.html doesn't pass in WebKit, since we properly handle the case where you don't create columns and don't establish a block formatting context in that case.

[Dave Hyatt]

I verified that the fix for 361551 does stop the assertion and resolve the performance issue with this test case, so:

\0/

[Dave Hyatt]

Created attachment 229815 [details]
Patch

[WebKit Commit Bot]

Attachment 229815 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderBlock.cpp:1475:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dave Hyatt]

I didn't mean to include tall-float1.html, so just ignore that part.

[Enrica Casucci]

Comment on attachment 229815 [details]
Patch

Looks good.

[Dave Hyatt]

Landed in r167602. Note EWS is going to choke on tall-float1.html, but I didn't land that, so it should be fine.