Open Bug 90723 Opened 23 years ago Updated 2 years ago

Concatenate table frames to improve performance

Categories

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

defect

Tracking

()

Future

People

(Reporter: bratell, Unassigned)

References

()

Details

(Keywords: perf, testcase, Whiteboard: [awd:tbl])

I was studying the table stress case in Quantify and noticed some things about 
all the frames.

Let's see if I have gotten it right...

A table is a TableFrame inside a TableOuterFrame
  In the TableFrame there are TableRowFrames, one for each row
    In each TableRowFrame there is one TableCellFrame for each cell
    Each TableCellFrame has a BoxFrame inside it.
    In this example there are a LineBoxFrame inside each BoxFrame
  
There are also TableColFrames which I don't understand. Where are they 
displayed? What children do they have?

Anyway, if this is correct, why have two frames, a TableCellFrame and a BoxFrame 
for each frame? Why not put the two into one? That should (for the example URL) 
save 10000 memory allocations and probably ~0.5 MB of memory. Even if they must 
be seperate frames for some reason, I see no reason not to allocate them 
together in one chunk. 

For the table stress case, frame creation is 20% of the page load time and 40% 
of that time are raw memory allocations. Now, that is done through an arena so 
maybe the gain won't be that big but it should help some at least.

Anyway 40242 frames for a 100x100 table seems like alot.
Keywords: perf
Blocks: 54542
Summary: Concatenate frames to improve performance → Concatenate table frames to improve performance
You mean BlockFrame, not BoxFrame, right?

Is the allocation of frames expensive?  They should all be allocated out of the
PresShell's arena.
Ah, yes, that should be BlockFrame.

The per-frame-cost is not that big. As for allocation, I don't think it can 
be any faster. The problem is that there are so many of them. For the table 
stress case (my favourite subject these days) it's ~100 ms to allocate the tens 
of thousands of frames. 

The real cost of frame creation is the initialization and style resolutions, but 
this is still only a couple of percent of the total time. Sadly, I think we have 
to cut these percents one by one to be faster. I don't think there are any low 
hanging fruit left (maybe bug 90503), but lots and lots of percent chasing.

Still I would rank this as one the lowest priority perf bugs I've filed this 
week. (bug 90503 is the big one)
just for the record, this testcase page rendered for me great, and quite speedy 
too!  What else is there for this bug?
Whiteboard: [awd:tbl]
Not as speedy as it could be. Frame construction is one of the things slowing it 
down most and this bug was to remember that and see if the number of frames 
could be decreased.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.1
Target Milestone: mozilla1.1alpha → M1
Target Milestone: M1 → mozilla1.1alpha
Any new stats about the number of frames?
Moving to future P3 until a realistic milestone can be determined. I don't think 
the effort of combining nsTableCellFrame and nsBlockFrame for table cells is 
worth the cost right now. 
Priority: -- → P3
Target Milestone: mozilla1.1alpha → Future
I agree with karnaze's opinion about it.
Any other approach that sounds reasonable?
mass reassign to default owner
Assignee: karnaze → table
Status: ASSIGNED → NEW
QA Contact: amar → madhur
Target Milestone: Future → ---
Target Milestone: --- → Future
Blocks: 71668
Keywords: testcase
OS: Windows 2000 → All
Priority: P3 → --
Hardware: PC → All
Target Milestone: Future → ---
Priority: -- → P3
Target Milestone: --- → Future
There is a technical reason: if the cell content is smaller than the cell height the vertical alignment moves the internal block with respect to the cell frame. I don't see how this would be easily feasible with a block frame below a row other than positioning all blocks and having a very special cell border painting.

I think we should close this bug as wontfix
Assignee: layout.tables → nobody
QA Contact: madhur → layout.tables
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.