Closed Bug 356335 Opened 18 years ago Closed 18 years ago

[FIX]Switch cellmap to nsTArray

Categories

(Core :: Layout: Tables, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 9 obsolete files)

Once bug 351942 lands and nsTArray has a SafeElementAt function, I think cellmap could just switch to nsTArray<nsTArray<CellInfo*> > instead of the nsVoidArray stuff it uses now.
Boris, what are the advantages of this change? I mean the code "works" (somehow... grumble grumble). Would it make it faster? I am not very familiar with it, so I would like to get a little bit educated. Pointing to a doc url would be fine :-) .
The main advantages are better code readability and maintainability (fewer casts, etc), and hopefully fewer allocations (placement new for the inner arrays, instead of separate heap allocations).  There might be a codesize win; depends on how it goes and what the compiler does with the template.

I was planning to just do this once those two bugs land, so I'm not asking you to spend time on this past reviewing it.  ;)
Attached patch Work in progress (obsolete) — Splinter Review
This doesn't have RebuildConsideringRows or RebuildConsideringCells implemented.
Depends on: 357630
Filed bug 357630 on the nsTArray part of this.
No longer depends on: 357630
Depends on: 357630
Blocks: 348805
Attached patch With some tweaks (obsolete) — Splinter Review
Attachment #243149 - Attachment is obsolete: true
Blocks: 357729
Attached patch Proposed limited patch (obsolete) — Splinter Review
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attached patch Same, but diff -w (obsolete) — Splinter Review
Attachment #243153 - Attachment is obsolete: true
Attachment #243690 - Attachment is obsolete: true
Attached patch Including bug 357729, as diff -w (obsolete) — Splinter Review
So data:

The testcase in bug 348805 takes up about 600MB RAM without any patches and takes about 38 seconds to load.

With the "limited patch", it takes about 325MB of RAM and takes about 27 seconds to load.

With this patch, it takes about 234MB of RAM and takes about 23 seconds to load.

Most of the pageload time is still under nsTCellMap::AppendCell and I'm pretty sure things could be made more efficient there.

One drawback is that a stripped gklayout is about 10KB bigger with this patch than with the limited one.  I think this is survivable, and some further cleanup of the structure of the cellmap code can probably reduce it even more.

The other drawback is that this patch is a heck of a lot bigger.  ;)  So if you would prefer to review the other one instead and put off the non-heap-allocation to a separate patch in bug 357729, let me know.
Attachment #243692 - Flags: superreview?(bugmail)
Attachment #243692 - Flags: review?(bernd_mozilla)
>So if you would prefer to review the other one instead and put off the
>non-heap-allocation to a separate patch in bug 357729, let me know.
Yes I prefer to review the items separately in smaller chunks. 56k of  a patch thats already more than my yearly average patch review volume ;-)
Comment on attachment 243689 [details] [diff] [review]
Same, but diff -w

OK, let's start with this one then.  It's pretty self-explanatory.  I'm not sure how much of a win it'll be in terms of reducing the size of the other patch, though...  ;)
Attachment #243689 - Flags: superreview?(bugmail)
Attachment #243689 - Flags: review?(bernd_mozilla)
Attachment #243692 - Flags: superreview?(bugmail)
Attachment #243692 - Flags: review?(bernd_mozilla)
hmm, why do we need to cache the prescontext? We worked pretty hard to remove just them from all the frames. Since we always have the rgFrame couldn't one retrieve the presContext via the frame pointer?
I tried that, but it crashes.  Cellmap teardown happens in ~nsTableFrame, by which point all the frames around have been destroyed.  I could move it, but since I plan to get rid of the arena stuff in bug 357729 it didn't seem worth it.
Attached patch Updated to fix for bug 339129 (obsolete) — Splinter Review
Attachment #243688 - Attachment is obsolete: true
Attachment #243689 - Attachment is obsolete: true
Attachment #243691 - Attachment is obsolete: true
Attachment #243692 - Attachment is obsolete: true
Attachment #243689 - Flags: superreview?(bugmail)
Attachment #243689 - Flags: review?(bernd_mozilla)
Attached patch Same as diff -w (obsolete) — Splinter Review
Attachment #243948 - Flags: superreview?(bugmail)
Attachment #243948 - Flags: review?(bernd_mozilla)
Hmm, this patch uses extensively InsertSlots which seems to be not the thing that will get r+ bug 357630.
Well...  It might have a different name, but it'll still work the same way.  I'll make sure that I land that bug first, of course, and then rename things in this diff as needed.
Comment on attachment 243948 [details] [diff] [review]
Same as diff -w

seems OK to me
Attachment #243948 - Flags: review?(bernd_mozilla) → review+
Summary: Switch cellmap to nsTArray → [FIX]Switch cellmap to nsTArray
Comment on attachment 243948 [details] [diff] [review]
Same as diff -w

>Index: layout/tables/nsCellMap.cpp
>@@ -1750,19 +1747,31 @@ void nsCellMap::ExpandWithCells(nsTableC
>     // add the originating cell data and any cell data corresponding to row/col spans
>     for (PRInt32 rowX = aRowIndex; rowX <= endRowIndex; rowX++) {
>-      nsVoidArray* row = (nsVoidArray *)mRows.ElementAt(rowX);
>+      CellDataArray& row = mRows[rowX];
>+      // Pre-allocate all the cells we'll need in this array, setting
>+      // them to null.
>+      // Have to have the cast to get the template to do the right thing.
>+      if (!row.InsertSlots(aColIndex, endColIndex - aColIndex + 1,
>+                           (CellData*)nsnull) &&

What happens without the cast? Compile error or does the wrong thing? Should we fix this in nsTArray.

Looks fine otherwise, sr=me.
Attachment #243948 - Flags: superreview?(bugmail) → superreview+
> What happens without the cast? Compile error or does the wrong thing? Should we
> fix this in nsTArray.

Compile error, since "nsnull" is just "0" (not "(void*)0") and there's no nsCellMap* constructor that takes an int.  Yay templates.

I don't see how you could fix this in nsTArray....

Patch checked in.  Now let's see how big the diff for bug 357729 is.  ;)
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
This is updated to the right method name, per comment 18.
Attachment #243947 - Attachment is obsolete: true
Attachment #243948 - Attachment is obsolete: true
Attachment #247766 - Flags: review?(bernd_mozilla)
Comment on attachment 247766 [details] [diff] [review]
Fix orange after landing

yep, thats good enough for government use
Attachment #247766 - Flags: review?(bernd_mozilla) → review+
Depends on: 363239
Depends on: 363370
Depends on: 364150
Depends on: 364318
Depends on: 365185
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: