Bug 13431 - calcMinMaxWidth should be lazy.
Summary: calcMinMaxWidth should be lazy.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords:
Depends on:
Blocks: 13351 13440
  Show dependency treegraph
 
Reported: 2007-04-21 03:55 PDT by Dave Hyatt
Modified: 2007-04-25 12:30 PDT (History)
0 users

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2007-04-21 03:55:06 PDT
calcMinMaxWidth should occur as needed to answer the question of minwidth/maxwidth and not before then.
Comment 1 Dave Hyatt 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.
Comment 2 Dave Hyatt 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.
Comment 3 Dave Hyatt 2007-04-22 04:38:48 PDT
Created attachment 14134 [details]
Snapshot #2, tighten things up a bit. Optimize more.
Comment 4 Dave Hyatt 2007-04-22 22:05:47 PDT
Created attachment 14142 [details]
Passes all layout tests.

Ready to go I think.
Comment 5 Darin Adler 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.
Comment 6 Darin Adler 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.
Comment 7 Darin Adler 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()?
Comment 8 Darin Adler 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.
Comment 9 Darin Adler 2007-04-24 23:51:09 PDT
Created attachment 14169 [details]
round 2 of changes to reduce the patch size
Comment 10 Maciej Stachowiak 2007-04-25 00:08:34 PDT
Comment on attachment 14169 [details]
round 2 of changes to reduce the patch size

r=me
Comment 11 Darin Adler 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.
Comment 12 Darin Adler 2007-04-25 00:40:07 PDT
Created attachment 14170 [details]
more reduced patch, passes layout tests (code 99% by Hyatt, not me)
Comment 13 Darin Adler 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
Comment 14 Dave Hyatt 2007-04-25 12:30:37 PDT
Fixed.