Closed Bug 12592 Opened 26 years ago Closed 26 years ago

IR_StyleChanged of table cells triggers full table reflow.

Categories

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

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: 26 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.