Closed Bug 560613 Opened 14 years ago Closed 7 years ago

Renumbering all rows on every row frame removal is slow


(Core :: Layout: Tables, defect)

Not set



Tracking Status
blocking2.0 --- -
status2.0 --- wanted


(Reporter: bzbarsky, Unassigned)


(Blocks 1 open bug)


(Keywords: perf)

It might be better to just set a flag that says we need to renumber rows and renumber them if/when we really need to, at least if nothing in table frame manipulation depends on these numbers.  See bug 559396 for the issues the current behavior causes: every row frame removal calls nsTableRowGroupFrame::AdjustRowIndices which walks all row frames.
Blocks: 559396
blocking2.0: --- → ?
roc, do we have anyone who might be able to work on this?
Lots of people *could* work on this, it's a question of priorities...
OK.  Do we have a general layout perf tracker?
A tracking bug? Not that I know of.
OK, filed bug 561937.
>every row frame removal calls
>nsTableRowGroupFrame::AdjustRowIndices which walks all row frames.
Boris are you saying that nsTableFrame::RemoveRows is called with single rows for every row that gets removed?
we do as  nsTableRowGroupFrame::RemoveFrame shows
> Boris are you saying that nsTableFrame::RemoveRows is called with single rows
> for every row that gets removed?

In the testcase in bug 559396, yes.  That testcase sets some (non-contiguous) subset of the rows to display:none, and therefore we remove the rowframes corresponding to those rows one at a time.
I think regardless of the ordering we are only removing one frame at a time.

Further the cellmap manipulation heavily relies on the correct rowindex, so before we call into cellmap manipulation we need to be sure that the rowindex is correct.

1. step we call remove rows the rowIndex of the row to be removed is correct we defer adjustRowIndices.
2. we call a second time remove rows, the row index is unknown to be correct we call the deferred AdjustRowIndex before we can remove as we will otherwise do bad things in the cellmap.

==> performance gain = 0

What would really help is to remove more than one frame at a time.
Did this get fixed in bug 1285874?
Flags: needinfo?(npancholi)
Depends on: 1285874
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #10)
> Did this get fixed in bug 1285874?

Yes, I believe so.
With the fix for Bug 1285874, instead of calculating all the new indices at the time of row deletion (as AdjustRowIndices was doing) we just add the index of the newly deleted row to a map. The final index is then recalculated on the basis of the indices in the map when GetRowIndex is called. 
The only place where AdjustRowIndices is now called is in InsertRows ->
Flags: needinfo?(npancholi)
Closed: 7 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.