23714 - remove redundant style context resolution for table rows

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Chris Waterson]

Filing a bug just so we don't forget. Remove the redundant style resolution that
we discovered while profiling tree scrolling today.

Comment 1

[David Hyatt]

cc'ing karnaze and troy.  The issue here is that during table frame construction
we resolve style contexts more than once.  Since this is a very expensive
operation, it shows up on the profiling radar.

I was thinking of adding an optional additional parameter to COnstructFrame,
a child style context... if not null, then ConstructFrame wouldn't call
ResolveStyleContext again.

Sound good?

Comment 2

[karnaze (gone)]

Related to this, we should convert all of the reflow code to use the new
pointers that troy added to the reflow state (e.g. mStyleDisplay,
mStylePosition, mStyleSpacing).

Comment 3

[troy]

Sounds okay to me

Comment 4

[leger]

BULK MOVE: Changing component from XUL to XP Toolkit/Widgets: XUL.  XUL 
component will be deleted.

Comment 5

[Paul MacQuiddy]

*IGNORE* - massive spam changing open XPToolkit bug's QA contact to
[e-mail]

Comment 6

[David Hyatt]

Giving this to karnaze.  Fix at your leisure, Chris.  Bug isn't high priority.

Comment 7

[karnaze (gone)]

mass move to m16

Comment 8

[buster]

marking perf, cc'ing some folks who might have time and motivation to do this.  
Style resolution is expensive, and style contexts are large.

Comment 9

[karnaze (gone)]

Marking mozilla0.9. This is related to bug 54542.

Comment 10

[karnaze (gone)]

Moving to mozilla0.8.1

Comment 11

[karnaze (gone)]

Attachment: patch to stop resolving style on irrelevant whitespace

Comment 12

[rbs]

+PRBool 
+NeedFrameFor(nsIFrame*   aParentFrame,
+             nsIContent* aChildContent) 
+{
+  // don't create a whitespace frame if aParentFrame doesn't want it
+  if (IsOnlyWhiteSpace(aChildContent)) {
+    nsFrameState state;
+    aParentFrame->GetFrameState(&state);
+    if (NS_FRAME_EXCLUDE_IGNORABLE_WHITESPACE & state) {
+      return PR_FALSE;
+    }
+  }
+  return PR_TRUE;
+}

PERF: I would suggest swaping, and testing the state bit first. It is
cheaper and would be set only on comparatively few frames, so leading to
a rapid exit, with no QI for nsITextContent.

Comment 13

[Chris Waterson]

Looks good modulo what rbs said. Also, make NeedsFrameFor() be ``static'', since
it's not used outside this file. Do that, and [s]r=waterson

Comment 14

[karnaze (gone)]

Attachment: better patch incorporating reviewers' suggestions

Comment 15

[karnaze (gone)]

Attachment: ignore previous patch, use this one

Comment 16

[karnaze (gone)]

The patch is in. This helps bug 54542 a bit.