Bug 13431

Summary:

calcMinMaxWidth should be lazy.

Product:

WebKit

Reporter:

Dave Hyatt <hyatt>

Component:

Layout and Rendering

Assignee:

Dave Hyatt <hyatt>

Status:

RESOLVED FIXED

Severity:

Normal

Priority:

Version:

523.x (Safari 3)

Hardware:

Mac

OS:

OS X 10.4

Bug Depends on:

Bug Blocks:

13351, 13440

Attachments:

Description	Flags
Patch to make minmaxwidth calculations happen lazily	none
Snapshot #2, tighten things up a bit. Optimize more.	none
Passes all layout tests.	none
first reduced patch (still passes all layout tests)	none
round 2 of changes to reduce the patch size	none
more reduced patch, passes layout tests (code 99% by Hyatt, not me)	darin: review+

Dave Hyatt

Reported 2007-04-21 03:55:06 PDT

calcMinMaxWidth should occur as needed to answer the question of minwidth/maxwidth and not before then.

Attachments
Patch to make minmaxwidth calculations happen lazily (112.85 KB, patch) 2007-04-22 03:10 PDT, Dave Hyatt	no flags	Details Formatted Diff Diff
Snapshot #2, tighten things up a bit. Optimize more. (125.63 KB, patch) 2007-04-22 04:38 PDT, Dave Hyatt	no flags	Details Formatted Diff Diff
Passes all layout tests. (154.86 KB, patch) 2007-04-22 22:05 PDT, Dave Hyatt	no flags	Details Formatted Diff Diff
first reduced patch (still passes all layout tests) (80.18 KB, patch) 2007-04-24 23:20 PDT, Darin Adler	no flags	Details Formatted Diff Diff
round 2 of changes to reduce the patch size (35.21 KB, patch) 2007-04-24 23:51 PDT, Darin Adler	no flags	Details Formatted Diff Diff
more reduced patch, passes layout tests (code 99% by Hyatt, not me) (69.12 KB, patch) 2007-04-25 00:40 PDT, Darin Adler	darin: review+	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Dave Hyatt

Comment 1 2007-04-22 03:10:17 PDT

Created attachment 14133 [details] Patch to make minmaxwidth calculations happen lazily This patch passes all layout tests and makes minmax recalc happen lazily. This involved a substantial amount of shuffling of code that had sort of randomly glommed on to these methods to do other work (first-letter, counters, list markers, etc.). Results should be excellent as long as you don't use a table. Once you start using tables, you're sunk of course.

Dave Hyatt

Comment 2 2007-04-22 03:27:36 PDT

Patch has no real effect on the PLT. Too many tables I guess. I'm going to profile just to make sure there isn't a speedup coupled to a slowdown that might be masking gains.

Dave Hyatt

Comment 3 2007-04-22 04:38:48 PDT

Created attachment 14134 [details] Snapshot #2, tighten things up a bit. Optimize more.

Dave Hyatt

Comment 4 2007-04-22 22:05:47 PDT

Created attachment 14142 [details] Passes all layout tests. Ready to go I think.

Darin Adler

Comment 5 2007-04-22 22:10:44 PDT

Comment on attachment 14142 [details] Passes all layout tests. It looks to me like the cssstyleselector.cpp has nothing to do with the rest of this patch. It's too late now, I guess, but I would have suggested making the name change separate from the laziness change. It would make it a hell of a lot easier to read the patch. I started reviewing, but didn't have the staying power to read and understand the whole thing. So not reviewed yet by me.

Darin Adler

Comment 6 2007-04-24 23:20:44 PDT

Created attachment 14168 [details] first reduced patch (still passes all layout tests) I landed a first set of safe changes and reduced the patch to this. I can do this again and land a second set.

Darin Adler

Comment 7 2007-04-24 23:21:08 PDT

Questions for Hyatt: Why does RenderTable still have an updateFirstLetter() function? Should it be removed, or should RenderBlock's updateFirstLetter() be changed to a virtual function? What kind of test would show the difference? Why is updateFirstLetter() called in both RenderBlock::layout() and RenderBlock::calcPrefWidths()?

Darin Adler

Comment 8 2007-04-24 23:50:00 PDT

Why isn't RenderObject::containingBlock a virtual function? Instead it has code that does isTableCell() and isRenderView() at the start of the function, which is more expensive than just being a virtual function in the first place.

Darin Adler

Comment 9 2007-04-24 23:51:09 PDT

Created attachment 14169 [details] round 2 of changes to reduce the patch size

Maciej Stachowiak

Comment 10 2007-04-25 00:08:34 PDT

Comment on attachment 14169 [details] round 2 of changes to reduce the patch size r=me

Darin Adler

Comment 11 2007-04-25 00:09:40 PDT

Comment on attachment 14169 [details] round 2 of changes to reduce the patch size Landed now, so clearing the reviewed flag.

Darin Adler

Comment 12 2007-04-25 00:40:07 PDT

Created attachment 14170 [details] more reduced patch, passes layout tests (code 99% by Hyatt, not me)

Darin Adler

Comment 13 2007-04-25 12:01:38 PDT

Comment on attachment 14170 [details] more reduced patch, passes layout tests (code 99% by Hyatt, not me) Comments from our review: Need to leave updateFirstLetter virtual (so we should dump RenderBlock.h). Also should remove the mutable. r=me

Dave Hyatt

Comment 14 2007-04-25 12:30:37 PDT

Fixed.

Note You need to log in before you can comment on or make changes to this bug.