Closed Bug 12592 Opened 21 years ago Closed 21 years ago

IR_StyleChanged of table cells triggers full table reflow.

Categories

(Core :: Layout: Tables, defect, P1, major)

x86
Other
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: hyatt, Assigned: troy)

Details

(Whiteboard: [code] 9/30: Requested verification by assigned engineer or reporter)

This function is pasted from nsTableCellFrame.cpp.  The way it is defined is
killing tree widget performance.

NS_METHOD nsTableCellFrame::IR_StyleChanged(nsIPresContext&
aPresContext,
                                            nsHTMLReflowMetrics&
aDesiredSize,
                                            const nsHTMLReflowState&
aReflowState,
                                            nsReflowStatus&          aStatus)
{
  nsresult rv = NS_OK;
  // we presume that all the easy optimizations were done in the
nsHTMLStyleSheet before we
were called here
  // XXX: we can optimize this when we know which style attribute changed
  nsTableFrame* tableFrame=nsnull;
  rv = nsTableFrame::GetTableFrame(this, tableFrame);
  if ((NS_SUCCEEDED(rv)) && (nsnull!=tableFrame))
  {
    tableFrame->InvalidateCellMap();
    tableFrame->InvalidateFirstPassCache();
  }

  return rv;
}

On any style change, you are invalidating the cell map and first pass cache.
This causes the entire table to reflow on any style change that affects a cell
(even simple things like, oh, say, selection).

This is killing the tree widget's selection performance (the only thing that's
changing is the background color of cells, but the entire tree gets reflowed
because of this).

This happens because the table code just following an incremental question asks
if the table needs to be reflowed fully.  The NeedsReflow function in
nsTableFrame.cpp will return TRUE if any of four things are invalid.  Because
the table cell invalidated two of those four things, a full reflow occurs.

Argh!
Severity: normal → major
Priority: P3 → P2
Summary: IR_StyleChanged of table cells triggers full table reflow. → [dogfood] IR_StyleChanged of table cells triggers full table reflow.
I am probably going to override this function in nsTreeCellFrame.cpp and make
sure it invalidates nothing as a temporary workaround.  If/when you fix this,
please let me know (preferably in e-mail) so that I can remove my hack.

Thanks!
Priority: P2 → P1
Target Milestone: M10
The act of simple tree selection really is a good example of what's bad about
table reflow right now.

Consider the act of selecting five rows in a tree with six columns.  I have to
set the selected attribute on each row individually, and have no batching
capability.  Therefore I end up with a guaranteed minimum of 5 incremental
reflows.  Now each setting of an attribute causes six reflow commands to be
generated, one for each cell in the row.

I'm now up to a grand total of 30 reflow commands.  Each one of these reflow
commands goes into IR_StyleChanged, which does this invalidation.  I therefore
end up doing 30 full reflows of the tree.  30 full reflows!
Let's see:
1. why is changing the background color even going through a reflow? It seems
excessive to me

2. Steve wrote IR_StyleChanged(). Yes it needs optimizing, but so does all the
table incremental reflow code. I'm slowly working my way through it

3. you could stop using tables for a tree view. I was never really thrilled
about this anyway

4. why is one reflow command being generated per table cell? That seems pretty
busted. We might just as well tell the entire row that the style changed and
generate one reflow command

5. style changed probably needs the same changes that insert/append/remove went
through, where we  let the frame generate the reflow command. That way we can
use the dirty bits and have a decent chance at coalescing them down to just one
reflow command
Interesting.  The table code has a line at the top of its reflow method that
causes the tree to do a full reflow.

if ((NS_UNCONSTRAINEDSIZE == aReflowState.availableWidth) &&
      (this == (nsTableFrame *)GetFirstInFlow())) {
    InvalidateFirstPassCache();
}

Once this first pass cache is invalidated, the NeedsReflow() method (following
the incremental reflow) return PR_TRUE, and then the tree ends up getting
reflowed fully.

Also, the tree is a fixed table (table-layout: fixed) and therefore needs no
pass 1 reflow.  If that has to do with mFirstPassCache, then it seems like this
should not cause a full reflow for fixed tables, since no first pass is
required.
1. Ok, it's probably because the text in the cell is being changed to white and
may not have to do with the purple background.
2. Cool.
3. I would love to not use tables. I'm not thrilled with it either.
Unfortunately, I think it's too late to reverse the decision.
4. Probably because the style code figures out that each cell needs a style
changed reflow.
5. Yeah.
I don't see why the text changing to white is all that different from the
background color changing. Maybe Peter hasn't optimized that case yet?

As far as the code below. I think Chris added it rececntly to fix a bug. I don't
remember why, though.

if ((NS_UNCONSTRAINEDSIZE == aReflowState.availableWidth) &&
      (this == (nsTableFrame *)GetFirstInFlow())) {
    InvalidateFirstPassCache();
}
Yes, I added it to fix a bug involving nested tables. And it is necessary until
incremental reflow is more solid especially involving nested tables. Keep in
mind that the only time a table gets reflowed with unconstrained width is if it
is nested. So, I think this is probably not the real cause of Hyatt's problems.
So I'm seeing an IR_StyleChanged reflow when the only rule that gets matched
upon selection is this:

treeitem[selected="true"] > treerow {
 color: white;
 background-color: #666699;
}

All I'm changing are colors and I'm seeing a reflow.
Summary: [dogfood] IR_StyleChanged of table cells triggers full table reflow. → IR_StyleChanged of table cells triggers full table reflow.
Downgrading this, since i'm of the opinion that a miscomputation of style hints
is the more serious problem.
Status: NEW → ASSIGNED
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Marking this FIXED, because the table cell frame code has been changed to no
longer invalidate the cell map and first pass cache.

Now what happens is that the table cell frame's Reflow() code handles the style
changed incremental reflow command by reflowing the area frame with the
eReflowReason_StyleChange reflow reason, and does nor invalidate the cell map or
first pass cache

The row frame IR_TargetIsChild() code has also been optimized. Now after
reflowing the table cell if we determine that the columns may have been
invalidated we see whether it's a fixed-layout table in which case we skip the
unconstrained width reflow

If you have concerns abut why we're generating an incremental reflow (or how
many), please file them as separate bug reports
Whiteboard: [code]
Whiteboard: [code] → [code] 9/30: Requested verification by assigned engineer or reporter
troy or hyatt: I am unable to verify this bug fixed. Could one of you please
mark it as so? Thanks.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.