12592 - IR_StyleChanged of table cells triggers full table reflow.

Status: VERIFIED FIXED

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

Description

[David Hyatt]

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!

Comment 1

[David Hyatt]

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!

Comment 2

[David Hyatt]

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!

Comment 3

[troy]

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

Comment 4

[David Hyatt]

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.

Comment 5

[David Hyatt]

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.

Comment 6

[troy]

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();
}

Comment 7

[karnaze (gone)]

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.

Comment 8

[David Hyatt]

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.

Comment 9

[David Hyatt]

Downgrading this, since i'm of the opinion that a miscomputation of style hints
is the more serious problem.

Comment 10

[troy]

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

Comment 11

[Christine Hoffman]

troy or hyatt: I am unable to verify this bug fixed. Could one of you please
mark it as so? Thanks.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2829c35d89e
[components] Upgrade version of nimbus-fml to 93.8.0 (https://github.com/mozilla-mobile/android-components/pull/12592)