Bug 131801

Summary: [New Multicolumn] Column balancing is much slower than the old multi-column layout on float-multicol.html
Product: WebKit Reporter: Dave Hyatt <hyatt>
Component: Layout and RenderingAssignee: Dave Hyatt <hyatt>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, glenn, jonlee, kondapallykalyan, mstensho, webkit-bug-importer, WebkitBugTracker
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch enrica: review+

Description Dave Hyatt 2014-04-17 13:10:26 PDT
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.
Comment 1 Radar WebKit Bug Importer 2014-04-17 23:13:09 PDT
<rdar://problem/16656291>
Comment 2 Morten Stenshorne 2014-04-18 01:49:14 PDT
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/
Comment 3 Dave Hyatt 2014-04-21 11:44:53 PDT
(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.
Comment 4 Dave Hyatt 2014-04-21 11:46:44 PDT
I verified that the fix for 361551 does stop the assertion and resolve the performance issue with this test case, so:

\0/
Comment 5 Dave Hyatt 2014-04-21 11:51:25 PDT
Created attachment 229815 [details]
Patch
Comment 6 WebKit Commit Bot 2014-04-21 11:52:29 PDT
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.
Comment 7 Dave Hyatt 2014-04-21 11:59:22 PDT
I didn't mean to include tall-float1.html, so just ignore that part.
Comment 8 Enrica Casucci 2014-04-21 12:01:00 PDT
Comment on attachment 229815 [details]
Patch

Looks good.
Comment 9 Dave Hyatt 2014-04-21 12:06:08 PDT
Landed in r167602. Note EWS is going to choke on tall-float1.html, but I didn't land that, so it should be fine.